[PATCH] libcpp: Implement -Wbidi-chars for CVE-2021-42574 [PR103026]

[Marek Polacek]

On Tue, Nov 30, 2021 at 09:38:57AM +0100, Stephan Bergmann wrote:
> On 15/11/2021 18:28, Marek Polacek via Gcc-patches wrote:
> > On Mon, Nov 08, 2021 at 04:33:43PM -0500, Marek Polacek wrote:
> > > Ping, can we conclude on the name?   IMHO, -Wbidirectional is just fine,
> > > but changing the name is a trivial operation.
> > 
> > Here's a patch with a better name (suggested by Jonathan W.).  Otherwise no
> > changes.
> > 
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > 
> > -- >8 --
> >  From a link below:
> > "An issue was discovered in the Bidirectional Algorithm in the Unicode
> > Specification through 14.0. It permits the visual reordering of
> > characters via control sequences, which can be used to craft source code
> > that renders different logic than the logical ordering of tokens
> > ingested by compilers and interpreters. Adversaries can leverage this to
> > encode source code for compilers accepting Unicode such that targeted
> > vulnerabilities are introduced invisibly to human reviewers."
> > 
> > More info:
> > https://nvd.nist.gov/vuln/detail/CVE-2021-42574
> > https://trojansource.codes/
> > 
> > This is not a compiler bug.  However, to mitigate the problem, this patch
> > implements -Wbidi-chars=[none|unpaired|any] to warn about possibly
> > misleading Unicode bidirectional characters the preprocessor may encounter.
> > 
> > The default is =unpaired, which warns about improperly terminated
> > bidirectional characters; e.g. a LRE without its appertaining PDF.  The
> > level =any warns about any use of bidirectional characters.
> > 
> > This patch handles both UCNs and UTF-8 characters.  UCNs designating
> > bidi characters in identifiers are accepted since r204886.  Then r217144
> > enabled -fextended-identifiers by default.  Extended characters in C/C++
> > identifiers have been accepted since r275979.  However, this patch still
> > warns about mixing UTF-8 and UCN bidi characters; there seems to be no
> > good reason to allow mixing them.
> 
> I wonder what the rationale is to warn about UCNs, like in
> 
> >         aText = u"\u202D" + aText;
> 
> (as found in the LibreOffice source code).

Is this line mixing a UCN and a UTF-8?  Or is it just that you're
prepending a LRO to aText?  We warn because the LRO is not "closed"
in the context of its string literal, which was part of the Trojan
source attack.  So "\u202D ... \u202C" would not warn.

I'm not sure what workaround I could offer.  Maybe provide an option not to
warn about UCNs at all, though even that is potentially dangerous -- while
you can see UCNs in the source code, if you print strings containing them,
they won't be visible anymore.

Marek