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

[Stephan Bergmann <sbergman@redhat.com>]

On 30/11/2021 14:26, Marek Polacek wrote:
> 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.

I'm not sure what you mean with "mixing a UCN and a UTF-8", but what the 
code apparently does is programmatically constructing a larger piece of 
text by prepending LRO to an existing piece of text.

My understanding is that Trojan Source is concerned with presentation of 
program source code and not with properties of Unicode text constructed 
during the execution of such a program, and from the documentation 
quoted above I understand that -Wbidi-chars is meant to address Trojan 
Source, so I don't understand why you're concerned here with what 
happens "if you print strings containing [UCNs in the source code]".

Short of a source code viewer that interprets UCNs in C/C++ source code 
and renders them in the same way as their corresponding Unicode 
characters, I don't think that UCNs are relevant for Trojan Source, and 
don't understand why -Wbidi-chars would warn about them.

(Also, I noticed that it doesn't work to silence -Werror=bidi-chars= with a

> #pragma GCC diagnostic ignored "-Wbidi-chars"

?)