{"thread":{"id":"56825","subject":"Re: b4: unicode control characters -- warn or remove?","startedAt":"2021-11-01T19:09:10Z","lastAt":"2021-11-02T14:09:21Z","messageCount":7,"participants":["Eric Wong","Konstantin Ryabitsev","Ævar Arnfjörð Bjarmason","Pavel Machek"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"440244","messageId":"20211101190905.M853114@dcvr","threadId":"56825","inReplyTo":"20211101175020.5r4cwmy4qppi7dis@meerkat.local","subject":"Re: b4: unicode control characters -- warn or remove?","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-11-01T19:09:05Z","receivedAt":"2021-11-01T19:09:10Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:\n> Hi, all:\n> \n> Per exhibit a, what should we do in the situation where we discover unicode\n> control characters in an email?\n> \n> 1. Warn and strip these chars out, because they are extremely unlikely to be\n>    doing anything legitimate in the context of a patch (unless someone is\n>    sending patches for docs actually written in RTL languages)\n> 2. Warn and error out, refusing to produce an mbox\n> 3. Just warn and produce an mbox anyway\n> \n> I'd normally do #3, but with many people piping things to git-am, I'm not sure\n> if it's the safest choice.\n> \n> Exibit a: https://lwn.net/Articles/874546/\n\n+Cc: git@vger\n\nIMHO, defense for this belongs in git-am (which already checks\nthings like whitespace).\n"},{"id":"440254","messageId":"20211101191723.vnsqrsx2jcw2nd2q@meerkat.local","threadId":"56825","inReplyTo":"20211101190905.M853114@dcvr","subject":"Re: b4: unicode control characters -- warn or remove?","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2021-11-01T19:17:23Z","receivedAt":"2021-11-01T19:17:27Z","isPatch":false,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"On Mon, Nov 01, 2021 at 07:09:05PM +0000, Eric Wong wrote:\n> > Per exhibit a, what should we do in the situation where we discover unicode\n> > control characters in an email?\n> > \n> > 1. Warn and strip these chars out, because they are extremely unlikely to be\n> >    doing anything legitimate in the context of a patch (unless someone is\n> >    sending patches for docs actually written in RTL languages)\n> > 2. Warn and error out, refusing to produce an mbox\n> > 3. Just warn and produce an mbox anyway\n> > \n> > I'd normally do #3, but with many people piping things to git-am, I'm not sure\n> > if it's the safest choice.\n> > \n> > Exibit a: https://lwn.net/Articles/874546/\n> \n> +Cc: git@vger\n> \n> IMHO, defense for this belongs in git-am (which already checks\n> things like whitespace).\n\nI agree, but even if that is implemented in git, we'll still probably want to\ncatch this on the b4 side of things until everyone uses the git client where\nthat's handled natively.\n\n-K\n"},{"id":"440265","messageId":"211101.86bl333als.gmgdl@evledraar.gmail.com","threadId":"56825","inReplyTo":"20211101190905.M853114@dcvr","subject":"Re: b4: unicode control characters -- warn or remove?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-01T20:02:34Z","receivedAt":"2021-11-01T20:05:55Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 01 2021, Eric Wong wrote:\n\n> Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:\n>> Hi, all:\n>> \n>> Per exhibit a, what should we do in the situation where we discover unicode\n>> control characters in an email?\n>> \n>> 1. Warn and strip these chars out, because they are extremely unlikely to be\n>>    doing anything legitimate in the context of a patch (unless someone is\n>>    sending patches for docs actually written in RTL languages)\n>> 2. Warn and error out, refusing to produce an mbox\n>> 3. Just warn and produce an mbox anyway\n>> \n>> I'd normally do #3, but with many people piping things to git-am, I'm not sure\n>> if it's the safest choice.\n>> \n>> Exibit a: https://lwn.net/Articles/874546/\n>\n> +Cc: git@vger\n>\n> IMHO, defense for this belongs in git-am (which already checks\n> things like whitespace).\n\nIt checks whitespace because that's something that's commonly a source\nof patch corruption. I'm not adverse to adding this to core.whitespace,\nbut trying to catch malicious injected code seems like a rather big\nexpansion of its scope, particularly since:\n\n    \"[...]sending patches for docs actually written in RTL languages[...]\"\n\nOr just code? People write comment and even in their native languages,\nand not all projects are as anglo-centric as those hosted on kernel.org.\n\nI haven't checked what the overlap is between solving this issue & i18n\nsupport, but we definitely should not be assuming that git's only using\nby kernel.org users & similar, even something as relatively obscure as\ngit-am.\n"},{"id":"440268","messageId":"20211101202220.dlcebvckeoz6c26k@meerkat.local","threadId":"56825","inReplyTo":"211101.86bl333als.gmgdl@evledraar.gmail.com","subject":"Re: b4: unicode control characters -- warn or remove?","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2021-11-01T20:22:20Z","receivedAt":"2021-11-01T20:22:25Z","isPatch":false,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"On Mon, Nov 01, 2021 at 09:02:34PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> It checks whitespace because that's something that's commonly a source\n> of patch corruption. I'm not adverse to adding this to core.whitespace,\n> but trying to catch malicious injected code seems like a rather big\n> expansion of its scope, particularly since:\n> \n>     \"[...]sending patches for docs actually written in RTL languages[...]\"\n> \n> Or just code? People write comment and even in their native languages,\n> and not all projects are as anglo-centric as those hosted on kernel.org.\n\nMy comment about docs was purely within the scope of the Linux kernel.\n\nI think the following would be a sane check:\n\n1. are there unicode control characters (CCs) present?\n2. are there other characters from RTL languages present in the same line?\n\nif both 1 && 2 are true, this is a legitimate use of Unicode CCs. If only 1 is\ntrue, then it's likely worth a warning.\n\nMaybe even relax #2 to just check for unicode characters above a certain\nbarrier where RTL languages live. I think everyone will agree that if there\nare unicode CCs and no other unicode characters in that same line, it's likely\nnot a legitimate use of control characters.\n\n-K\n"},{"id":"440270","messageId":"20211101204914.GA16445@duo.ucw.cz","threadId":"56825","inReplyTo":"20211101202220.dlcebvckeoz6c26k@meerkat.local","subject":"Re: b4: unicode control characters -- warn or remove?","fromName":"Pavel Machek","fromEmail":"pavel@ucw.cz","sentAt":"2021-11-01T20:49:14Z","receivedAt":"2021-11-01T20:51:05Z","isPatch":false,"sender":{"key":"pavel@ucw.cz","avatar":null},"body":"Hi!\n\n> > It checks whitespace because that's something that's commonly a source\n> > of patch corruption. I'm not adverse to adding this to core.whitespace,\n> > but trying to catch malicious injected code seems like a rather big\n> > expansion of its scope, particularly since:\n> > \n> >     \"[...]sending patches for docs actually written in RTL languages[...]\"\n> > \n> > Or just code? People write comment and even in their native languages,\n> > and not all projects are as anglo-centric as those hosted on kernel.org.\n> \n> My comment about docs was purely within the scope of the Linux kernel.\n> \n> I think the following would be a sane check:\n> \n> 1. are there unicode control characters (CCs) present?\n> 2. are there other characters from RTL languages present in the same line?\n> \n> if both 1 && 2 are true, this is a legitimate use of Unicode CCs. If only 1 is\n> true, then it's likely worth a warning.\n> \n> Maybe even relax #2 to just check for unicode characters above a certain\n> barrier where RTL languages live. I think everyone will agree that if there\n> are unicode CCs and no other unicode characters in that same line, it's likely\n> not a legitimate use of control characters.\n\nIf you are worried about malicious patches, then it should be easy for\nattackers to add some RTL characters and escape the check...\n\nBest regards,\n\t\t\t\t\t\t\t\tPavel\n-- \nhttp://www.livejournal.com/~pavelmachek\n"},{"id":"440272","messageId":"20211101210259.2patkw62rkemdqlt@meerkat.local","threadId":"56825","inReplyTo":"20211101204914.GA16445@duo.ucw.cz","subject":"Re: b4: unicode control characters -- warn or remove?","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2021-11-01T21:02:59Z","receivedAt":"2021-11-01T21:03:10Z","isPatch":false,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"On Mon, Nov 01, 2021 at 09:49:14PM +0100, Pavel Machek wrote:\n> > I think the following would be a sane check:\n> > \n> > 1. are there unicode control characters (CCs) present?\n> > 2. are there other characters from RTL languages present in the same line?\n> > \n> > if both 1 && 2 are true, this is a legitimate use of Unicode CCs. If only 1 is\n> > true, then it's likely worth a warning.\n> > \n> > Maybe even relax #2 to just check for unicode characters above a certain\n> > barrier where RTL languages live. I think everyone will agree that if there\n> > are unicode CCs and no other unicode characters in that same line, it's likely\n> > not a legitimate use of control characters.\n> \n> If you are worried about malicious patches, then it should be easy for\n> attackers to add some RTL characters and escape the check...\n\nWell, the point of this attack was to trick the reviewer into accepting code\nthat the compiler would treat differently (e.g. something that looked to be\ninside a comment block is actually outside of it).\n\nSo, if attackers include some actual RTL text, then the reviewer would no\nlonger be (as easily) tricked because there would be stuff other than just\ninvisible characters in the line of code.\n\nThis actually similar to how we treat unicode domains. Most browsers only\nallow unicode domains when the entire domain name consists of unicode\ncharacters. I suggest we take a similar approach.\n\n-K\n"},{"id":"440309","messageId":"20211102140907.d7turl5zhaxkcp7w@meerkat.local","threadId":"56825","inReplyTo":"20211101202220.dlcebvckeoz6c26k@meerkat.local","subject":"Re: b4: unicode control characters -- warn or remove?","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2021-11-02T14:09:07Z","receivedAt":"2021-11-02T14:09:21Z","isPatch":false,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"On Mon, Nov 01, 2021 at 04:22:20PM -0400, Konstantin Ryabitsev wrote:\n> I think the following would be a sane check:\n> \n> 1. are there unicode control characters (CCs) present?\n> 2. are there other characters from RTL languages present in the same line?\n> \n> if both 1 && 2 are true, this is a legitimate use of Unicode CCs. If only 1 is\n> true, then it's likely worth a warning.\n\nI implemented this solution in b4 master, so it should error out only when it\nfinds control characters without any \"other letter\" unicode character category\npresent in the same line (where Hebrew, Arabic, etc live). There's probably\nstill a way to take advantage of this, but hopefully it's a lot less trivial\nnow and less likely to go unnoticed by the reviewer.\n\nThe error message will point where it found the problem:\n\n\tWARNING: Message contains suspicious unicode control characters!\n\t\t\t Subject: [PATCH 1/2] SPI: Add SPI driver for Sunplus SP7021\n\t\t\t\tLine: + /* ‮ } ⁦if (isAdmin)⁩ ⁦ begin admins only */\n\t\t\t\t-----------^\n\t\t\t\tChar: RIGHT-TO-LEFT OVERRIDE (0x202e)\n\t\t\t If you are sure about this, rerun with the right flag to allow.\n\nOne can rerun with --allow-unicode-control-chars to override this.\n\n-K\n"}]}