• Hanno Böck's avatar
    GVariant text: fix scan of positional parameters · aead1c04
    Hanno Böck authored and Allison Karlitskaya's avatar Allison Karlitskaya committed
    The scanning to find the end of a positional parameter designator in
    GVariant text format (e.g. '%i') is currently broken in case the 'end'
    pointer is not specified.
    The scan is controlled by a somewhat complicated loop that needs to deal
    properly with cases like (123, %(ii)) [where '%(ii)' is to be taken
    together, but the final ')' not].
    This loop missed the case where a format string passed to
    g_variant_new_parsed() ended immediately after such a conversion, with a
    nul character.  In this case the 'end' pointer is NULL, so the only way
    we can find the end is by scanning for nul in the string.
    In case of g_variant_new_parsed() [which is what this code was designed
    to be used for], the bug is somewhat unlikely in practice: the only way
    that a valid text-form GVariant could ever contain a positional
    parameter replacement at the end of the string is if this positional
    parameter were the only thing being returned.  In that case, the user
    would likely have opted for a more direct approach.
    Unfortunately, this code is also active in the tokenisation phase of
    g_variant_parse(), before positional parameters are rejected as invalid
    for that case.  Anyone who calls this function with a nul-terminated
    string (and no end pointer) is vulnerable to a crash from malicious user
    input.  This can be seen, at the very least with many commandline tools:
      $ dconf write /x '%i'
      Segmentation fault
    We fix this problem by searching for the nul character in this case, in
    addition to comparing the end pointer.
    This problem is almost certainly limited to being able to cause crashes.
    The loop in question only performs reads and, in the security-sensitive
    case, the token will be quickly rejected after the loop is finished
    (since it starts with '%' and the 'app' pointer is unset).  This is
    further mitigated by the fact that there are no known cases of GVariant
    text format being used as part of a protocol at a privilege barrier.