Skip to content

Format string vulnerability in registration message override

Low
DeD1rk published GHSA-vf8w-xr57-hw87 Jun 12, 2024

Package

concrexit

Affected versions

< 53.0

Patched versions

53.0

Description

Summary

A user with event management privileges can provide their own registration message overrides, which support string formatting.
Python string formatting allows attribute traversal to leak variables from the python process.

Details

In concrexit/website/events/services.py, there are 2 paths that involve a .format call on a string that is user input:

return infos[cancel_status].format(fine=event.fine)
status_msg.format(
        fine=event.fine,
        pos=queue_pos,
        regstart=localize(timezone.localtime(event.registration_start)),
    )

This was introduced in #2458 and became accessible to users in #3617 .

Due to the specific types involved in this keyword formatting there is currently no path to impactful information leakage, but this is moreso a lucky coincedence than anything that should be relied upon.

In python, functions that are imported from a non-c-implementation source have __builtins__ and __globals__ attributes through which variables in the current python process can be leaked, such as secret keys. See https://docs.python.org/3/library/inspect.html
e.g. see random.SystemRandom.__init__.__globals__, or

def Hi():
    pass

print(dir(Hi))

In the case present in services.py, .format is called with Decimal, int and str for which (as far as I can tell) no path exists to these dangerous attributes, though object, type, tuple, mappingproxy and such can all be reached. If the c implementation of decimal were missing, it would fall back to a python implementation of it, which I think would lead to this vulnerability actually being able to leak globals.

In any case, this behaviour should be understood and probably replaced with something more secure, because python updates that affect the internals (which happens almost every major version) could unexpectedly lead this behaviour to become a high impact vulnerability. e.g. if subclasses became available via a property instead of requiring a call, this would again lead to a path to globals.

Sidenote: Using nonexistent names in the format string results in a KeyError, which I think is likely also not desired to be possible behaviour.

PoC

In https://thalia.nu/admin/events/event/[id]/change/, under Registration status messages, in any of the fields for which an example is given with format string replacement fields in it, enter something like {pos.__class__.__mro__[1].__doc__}. When you navigate to a page that displays the result of this, it should show 'The base class of the class hierarchy.\n\nWhen called, it accepts no arguments and returns a new featureless\ninstance that has no instance attributes and cannot be given any.\n', demonstrating that attribute traversal works. I've chosen the __doc__ attribute here because you perform some kind of html sanitization which removes the default representation of class objects, as these are of the form <class 'int'> which seems to be removed for looking like an html tag.

Impact

I would describe it as a low impact vulnerability currently, but potentially large impact in the future if unfixed. (leaking app secrets)

Recommendations

This section wasn't part of the provided template, but I do think it would be useful:
Avoid doing any string formatting on user-provided input, unless the replacement fields are strongly restricted, because python string formatting is quite powerful.
E.g. you could filter the content of the replacement fields to only allow exactly those keys that are used as kwargs in the call to .format and certainly no ., [ or ].
Even easier (and probably more elegant) would be to use a .replace based "safe formatting" function instead. (either write it yourself or use one that django provides)

Severity

Low

CVE ID

No known CVE

Weaknesses

Credits