Carriage return without line feed can be used to hide malicious code

[krtab]

The issue is basically in the title: rustc parses a carriage return ("\r") as a proper whitespace token. However, on some platforms/systems (for example cat in my linux terminal), the carriage return is interpreted as simply returning the cursor to the beginning of the line and overwriting what follows.

Hence the following file:

fn do_something_very_bad() {
     todo!()
}

fn main() {
     do_something_very_bad();\r    // No need to worry here, this looks very innocuous, I think nothing bad will happen
     println!("Oh hi! You should definitely run this very innocuous binary");
}

is displayed as

fn do_something_very_bad() {
     todo!()
}

fn main() {
     // No need to worry here, this looks very innocuous, I think nothing bad will happen
     println!("Oh hi! You should definitely run this very innocuous binary");
}

The security team and I agree that this does not qualify as a security issue.

In the past, we have added lint to safe-guard against CVE-2021-42574 (https://blog.rust-lang.org/2021/11/01/cve-2021-42574/), which is similar and was treated as a security issue. However to quote Pietro: "the main reason we treated [CVE-2021-42574] that way back then was that editors and code review tools didn't handle it at all, while most editors and code review sites seem to handle \r gracefully".

We may be opening a can of worm by trying to properly handle all Unicode messes that may lead to wrong displays of code across all possible systems allowing to read code, but I think it it nonetheless worth it to have the possibility of a discussion.

[ais523]

In addition to literal carriage returns, effectively the same problem can occur with various other terminal control codes. For example, ending a line with // ␛[1M (except using a literal ESC character rather than ␛) will cause the line to not show up in a terminal output, because ESC [ 1 M is an encoding of the "delete line" terminal command and thus the terminal deletes the line from the output. (Effectively the same trick can be done using string literals rather than comments, but the setup is more complicated because you have to hide the closing double quote.) Rust currently doesn't lint on literal ESC if it appears in a comment or string literal, but there are a very large number of interesting ways to abuse it.

CR is slightly more problematic than the other terminal control codes, though, because most terminal control codes are echoed literally by most programs, whereas some programs interpret CR as though it were a newline (which means that if you put a CR inside a line comment, some programs will show the text after the CR as being inside the comment and others will show the text as being outside it). In Rust, this could be exploitable by "ending" a line comment with a carriage return rather than an actual newline: some programs will treat the carriage return as a newline and show the next line of code as being the next line, but Rust interprets it as part of the same line, and thus there would be a line of code which visually appeared (in a code review program) to be part of the live code but was actually commented out. Both GitHub and play.rust-lang.org (the first two places I tried) appear to interpret a copy-and-pasted carriage return as though it were actually a newline, so this might not be a totally theoretical risk.

From a security point of view, if we consider it desirable to be able to view Rust code in a terminal and see something similar to what the actual code does, the safest thing would be to warn on any control code sequences other than horizontal tab (\u{8}), newline (\u{A}), carriage return immediately followed by newline (\u{D}\u{A}), and perhaps form feed (\u{C}) on a line by itself (which is an old-fashioned way of dividing a source file into logical "pages" but one that isn't completely unheard of). If nothing else, it's generally confusing to use such characters without escaping them. (I can just about imagine someone using graphics-rendition commands intentionally to create colored comments, but most editors wouldn't color them correctly, and even these could be abused to hide code by rendering it as, e.g., black-on-black.) Although invisible characters like left-to-right marks have both legitimate uses and problematic uses, they wouldn't be caught by such a check due to being considered formatting characters rather than control characters.

In any case, I think it's desirable to warn at least on carriage returns inside line comments (except for those immediately followed by a newline), because their behaviour is both non-obvious and non-useful. This more restricted lint should also probably complain about other non-newline vertical whitespace characters (it isn't obvious whether or not a line comment should end at a \u{85} NEXT LINE or \u{2028} LINE SEPARATOR character – rustc's answer is "no to both", but a great many programs will treat both as equivalent to newlines because they are defined to have newline-like semantics) and vertical control characters (vertical tabs and formfeeds).

@rustbot label +A-diagnostics +A-parser

[SpriteOvO]

Interesting.

[ais523]

T-lang were discussing carriage returns in another context recently (#148051 (comment) and https://hackmd.io/GiLB0VK7S52-WdVxMSnsBA#Stabilize-Frontmatter-rust148051), and brought up the idea of possibly rejecting all carriage returns which weren't part of a CR/LF pair (which is close to what this discussion is asking for). As such, I'm linking the comment because I think it's relevant to this issue.

[traviscross]

The next step on that would be putting up a draft PR that rejects CRs that survive the single-pass CRLF normalization so that we can crater run it. If interested in doing that, ping me with it.