Tcl Source Code

The attached test case crashes the latest tclsh I have handy (8.5.11)

This is derived from a test case that was sent to the Postgres security list.  The Postgres project is currently thinking of treating this as just a regular bug and not a security issue, because as far as we can tell the potential damage is limited to a null-pointer dereference.  But we thought we'd let you know about it first to see if you feel it needs to be treated as a security issue.  If so, we'll need to organize coordinated releases.

Here's the analysis I sent to the Postgres security list:

I looked into this, and find that the core of the problem is that
parseqatom() is not defending itself against a NULL result from
parsebranch().  See this bit of code:

    if (!(SEE('|') || SEE(stopper) || SEE(EOS)))
        t->right = parsebranch(v, stopper, type, s2, rp, 1);
    else
    {
        EMPTYARC(s2, rp);
        t->right = subre(v, '=', 0, s2, rp);
    }
    assert(SEE('|') || SEE(stopper) || SEE(EOS));
    t->flags |= COMBINE(t->flags, t->right->flags);

If parsebranch (or subre for that matter) returns NULL, the
t->right->flags reference will obviously crash.

It turns out that parsebranch usually returns a valid struct subre
pointer even after an error, but it has two "NOERRN()" macros
(essentially "if (error occurred) return NULL") in places where only
an out-of-memory or regex-too-complex error would trigger the error
response.  This example case is provoking the regex-too-complex error.

This bug is aboriginal in Spencer's code.  The reason we've not seen it
reported before appears to be (a) typical errors such as regex syntax
errors don't end up returning NULL from parsebranch, and (2) you don't
get to the vulnerable code in parseqatom unless the regex has been found
to contain "messy stuff" (capturing parens, back reference, or
short/long preference clash).

The attached patch fixes it.  The added error check in parseqatom() is
the necessary fix, the additional check in parsebranch() is just
intended to not let processing continue after we know there's an error.

I'm not sure whether we should treat this as a security problem or just
fix it and be done.  AFAICS there is no risk of anything worse than a
null pointer dereference (no possibility of privilege escalation or
information leakage).  We've been less than consistent in the past as to
how to deal with simple crash cases like this; some have been treated as
security issues, but many more have just been handled as ordinary bugs.


BTW, credit for finding this issue goes to Tomasz Karlik <[email protected]>

> Is Tom Lane (tgl) aware of the other recent regexp fix in Tcl?
> Tcl Bug 3604074.

I'll take a look, thanks for the pointer.

I tried to use [string repeat] but that leads to memory errors in stead of the expected error message.
Anyway, wrapping it over multiple lines works. I'm open to ideas for further improvmement.

Can't we craft that test case using [string repeat], etc.
instead of such an ugly literal?  Is that addition to the
contents the cause of fossil now thinking regexp.test
is a binary file?

Is Tom Lane (tgl) aware of the other recent regexp fix in Tcl?
Tcl Bug 3604074.

Agreed with the analysis. I see no reason to consider this a security bug as well.

Fixed now on core-8-4-branch, core-8-5-branch and trunk.

Many thanks!

File Added - 461005: regex-crash-fix.patch

File Added - 461004: tcl_crash.tcl