Add chapter on underhanded code (#232) #239

wanders · 2024-05-02T18:37:52Z

This change is

kbeyls

Thank you very much for this contribution @wanders !
The comments I've left are just very minor nit picks.
The text reads easily and explains the issues well.

The only other thing I'd like to add to this change is to add copyright info for your contribution. (We try to keep the copyright information up-to-date)
See the top of the book.md source file, which looks like this at the moment:

---
SPDX-License-Identifier: CC-BY-4.0
copyright:
  - SPDX-FileCopyrightText: Copyright 2021-2024 Arm Limited <[email protected]>
  - SPDX-FileCopyrightText: Copyright 2023 Bill Wendling <[email protected]>
  - SPDX-FileCopyrightText: Copyright 2023 Lucian Popescu <[email protected]>

Could you add another SPDX-FileCopyrightText line with the appropriate information?

Reviewable status: 0 of 2 files reviewed, 14 unresolved discussions (waiting on @wanders)

book.md line 2495 at r1 (raw file):

on code inspection. However the same applies also when the
underhanded behavior has been added by mistake - in fact the ideal
attack would also give the attacker the possibility to say it was a

s/a honest/an honest/

book.md line 2499 at r1 (raw file):

[@Wheeler2020] provides a more systematic view of underhanded code
than is given here. This chapter present some examples to give an

s/present/presents/

book.md line 2507 at r1 (raw file):

Probably the most classic example of underhanded code in C-style
languages is code that takes advantage of the fact that assignment

maybe "the assignment (=) and the equality (==) operator look very similar"?

book.md line 2537 at r1 (raw file):

added to the language [@pep572] (in python 3.8) the more visually
distinct "walrus operator" `:=` was used to avoid this risk of
confusing with comparison operator. Even in cases where it is an

s/with comparison operator/with the comparison operator/?

book.md line 2539 at r1 (raw file):

confusing with comparison operator. Even in cases where it is an
existing language the compiler can provide options to forbid risky
constructs for those how want and can opt in to it, essentially

s/how/who/?

book.md line 2544 at r1 (raw file):

## goto fail

The "goto-fail" bug, officially known as CVE-2014-1266, caused Apple

I think it may be useful to add a link to CVE-2014-1266, e.g. to https://nvd.nist.gov/vuln/detail/CVE-2014-1266.
What do you think?

book.md line 2577 at r1 (raw file):

At the time of they bug they didn't but nowadays both gcc and clang

replace the first "they" with "the".

book.md line 2578 at r1 (raw file):

At the time of they bug they didn't but nowadays both gcc and clang
have a `-Wmisleading-indentation` option that detects kind this

s/kind this/this kind of/

book.md line 2581 at r1 (raw file):

problem.

Also automatic formatting such as clang-format would help finding this

maybe s/formatting/formatting tools/?

book.md line 2588 at r1 (raw file):

"Trojan Source" attacks described by [@Boucher2023] is another way
underhanded code could be achieved by doing something that makes the
editor (or other thing that displays code to user) render the code in

s/to user/to the user/?

book.md line 2592 at r1 (raw file):

This is done by using unicode features. Support for bidirectional text
is one such feature, where it has special chars to mark regions of

I'm wondering if "characters" instead of "chars" would be more correct here?

book.md line 2597 at r1 (raw file):

they can make the text render in a way that makes a word look like it
is inside a comment but to the compiler which parses it in the order
it appears in the file it is after the comment.

maybe s/it appears in the file it is after the comment/it appears in the file in, sees it as after the comment/?

book.md line 2599 at r1 (raw file):

it appears in the file it is after the comment.

`gcc` provides `-Wbidi-chars` to detect usage of writing direction

Maybe it would be useful to also mention that the on the clang side, the decision was to not integrate the check in clang, but rather in clang-tidy, see https://bugs.chromium.org/p/llvm/issues/detail?id=11.

book.md line 2604 at r1 (raw file):

Another variant of "trojan source" is usage of homoglyphs, different
characters that looks similar or even identical (depending on
font). As an example unicode contains the character "Division Slash"

Maybe s/As an example/As an example,/?

wanders · 2024-05-03T13:42:27Z

Thanks, I'll try to carve out some time during the weekend to update this. Have not used reviewable.io before - I guess pushing amended commit with fixes to my branch is expected workflow?

kbeyls · 2024-05-03T13:47:33Z

Thanks, I'll try to carve out some time during the weekend to update this. Have not used reviewable.io before - I guess pushing amended commit with fixes to my branch is expected workflow?

That's great :)

If you prefer, you don't have to use reviewable.io; but we've found it is better at keeping track of and not loosing review comments, especially when there are further commits, or amended commits to the pull request.

Yes, pushing commits to your branch should update this pull request automatically.
There is no need to amend/change the commit you already have on your branch, we can do a "squash and merge" merge that will squash all extra commits done into a single commit when merging into mainline. But if you prefer to alter/amend your current commit and force push to your branch, that is fine too.

wanders

Updated according to comments (hopefully). Also fixed capitalization of GCC and Clang.

("squash and merge" is my least favorite way to merge things (as what has been reviewed is not exactly same thing as what gets merged). And also I wanted to try out reviewable.io's handling of history rewriting -- and it seems to be nice)

Reviewable status: 0 of 2 files reviewed, 14 unresolved discussions (waiting on @kbeyls)

book.md line 2495 at r1 (raw file):