Format string vulnerabilities (and more) in multiple safe curses wrapper crates.

[thomcc]

I've noticed potentially exploitable security vulnerabilities in the following crates[0], which can be triggered by users of these libraries without writing unsafe code.

I'm filing an issue rather than a PR since I don't know that I have time to see this through to it's conclusion, especially since multiple crates and several functions are involved. The FAQ indicates that this is okay.

pancurses

The crate https://crates.io/crates/pancurses exposes the functions Window::printw and Window::mvprintw as safe functions (I think this is all of them, but could be wrong), despite these passing an arbitrary rust str to into C code which expects to receive a printf-style format string. This can lead to a format string vulnerability.

I filed ihalila/pancurses#66 about this, shortly before filing this issue.

ncurses

The crate https://crates.io/crates/ncurses exposes the functions printw, mvprintw, and mvwprintw as safe functions, which have the same issues described above.

In jeaye/ncurses-rs#172 I raised this issue about printw (but missed that it wasn't just printw), which lead to that functions deprecation[1]. When writing this issue for the advisory-db, I noticed the existence of the other two functions. I don't see any other functions accepting format strings (the scanw functions, mercifully, are not exposed).

Additionally, while filing this bug, I noticed the following additional vulnerabilities or memory safety issues (I haven't filed issues in the ncurses crate's repository about these):

• instr is exposed, which has a buffer overflow (the curses instr function is somewhat similar to the gets function from the C stdlib, in that writes as much text that it wants into a lengthless buffer pointer).

  • EDIT: I've noticed that mvwinstr has the same problem, just not as directly.

• Several functions (1, 2, 3) write data from the terminal directly into a String's underlying buffer, when the data may not be valid UTF-8. I don't know if this can be exploited, but it's a memory safety issue nonetheless. Hopefully the members of this repository have some intuition as to the exploitability of this sort of problem.

• I have not done a very thorough look, these are just the most obvious problems to me. Ideally, someone more familiar with the curses library's API would look further.

---

[0]: For total clarity, I didn't look at any others. It seems plausible to me that other Rust curses libs have similar mistakes, since it's a very hard API to use safely. That said, I think these are the most popular ones (not that that makes things any better).

[1]: I don't really feel that deprecation is sufficient, as the function is still present (and not unsafe). Transitive dependency deprecations aren't reported, so rust code consuming this crate transitively could still be at risk. However, if the members of this repository disagree, then one fewer of the ncurses crates functions are problematic in the current release.

[tarcieri]

Thanks for reporting these @thomcc. We generally have format injection in-scope, and some of these sound potentially exploitable.

It seems like the ones with issues already reported upstream like ihalila/pancurses#66 and jeaye/ncurses-rs#172 already potentially warrant advisories.

For the others, I'd suggest reporting them upstream first as well, and then we can decide on a case-by-case basis which of them also deserve advisories (buffer overflows seem like an obvious yes, whereas the "memory unsafety" of incorrectly encoded Strings seems a little more questionable as to whether it's a practical security issue).

[thomcc]

Fine. I filed issues for the two new ones (IMO jeaye/ncurses-rs#172 is still arguably unresolved since the function is still present and usable in safe rust, but maybe the pings from this activity will fix that).

I also noticed that mvwinstr can also cause a buffer overflow just as easily as instr while doing this, and edited the above to note that. I would be entirely unsurprising if there are more problems...

We generally have format injection in-scope, and some of these sound potentially exploitable.

I'd... certainly hope you consider them in scope.

Format string vulnerabilities (speficially the kind for printf-family functions) are often just as bad as buffer overflows, since you can use %n to write to arbitrary stack memory (including the return address, the pointer of a variable that's about to get freed, etc...).

the "memory unsafety" of incorrectly encoded Strings seems a little more questionable as to whether it's a practical security issue

Yeah, that's why I mentioned that I wasn't exactly sure if someone could exploit that.

(Since it's String and not str it seems plausible to me that you could potentially trick the String to write past it's end without bounds checking if the last utf8 sequence claims to be longer than it actually is -- but that also might be pretty hard to arrange in practice)

[Shnatsel]

Wow, this does look serious. Thanks for bringing it up!

[Shnatsel]

The only way to handle this correctly is expose these functions as unsafe fn.

Deprecation is definitely insufficient because it's not transitive, and more importantly, people do not expect this to mean "using this may cause memory errors". That's exactly what unsafe fn communicates.

[thomcc]

The issues in ncurses-rs have been closed as the author does not believe that it matters that these APIs are fundamentally broken. Filed advisories.

[tarcieri]

Those issues are merged and have been assigned RUSTSEC-2019-0005 and RUSTSEC-2019-0006.

@thomcc I think we can close this issue now?

[thomcc]

SGTM

[dpc]

I'll have to rethink how to handle cases where there is no fix (and maybe never will be) in cargo-crev. For now I've just filled a rating: dangerous review for the latest versions of these crates.

[rhmcruiser]

Can someone point me to the reproducer program ? I would like to try it out

[kpcyrd]

@rhmcruiser reproducer can be found here: ihalila/pancurses#54