Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GnuPG parsing (re: Johnny You Are Fired) #2217

Closed
BjarniRunar opened this issue May 4, 2019 · 4 comments

Comments

@BjarniRunar
Copy link
Member

commented May 4, 2019

Regarding: https://github.com/RUB-NDS/Johnny-You-Are-Fired

Mailpile currently combines GnuPG's stderr and status-fd output streams into one. It has been demonstrated that the stderr stream can be manipulated to inject random data, thus confusing our parser and in some cases tricking the parser into thinking we got a valid signature from someone, when in fact it we did not.

This is a security issue for anyone relying on OpenPGP signatures, and I consider this a blocking issue for a 1.0 release.

The obvious solution here, is to separate the streams. This should be relatively easy on Linux (and the Mac), but there are cross-platform concerns since Windows treats forking and file descriptors differently, and Python's subprocess.Popen doesn't natively give us a third file descriptor to work with.

@BjarniRunar

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

Progress: GnuPG supports a flag --status-file, so we can just send the output to a temporary file instead of a file descriptor. Initial tests suggest this is only a minor change and works fine (on Linux). AFAIK there's no reason it shouldn't also work fine on other platforms.

More soon.

@lambdafu

This comment has been minimized.

Copy link

commented May 6, 2019

@BjarniRunar Sorry for not letting you know before publication! Somehow it slipped through the cracks, given that we found the Mailpile issue over half a year after disclosing the same vulnerabilities in other mail clients and gpg. It's good to protect against older gpg versions in the wild of course.

Here are the strategies I give in my blog:

  • Call gpg with --no-verbose to disable the attack.
  • Use a dedicated pipe for --status-fd, and do not share it with stderr.
  • If this is not easy (or even possible) due to the framework or target platform, consider --batch --log-file FILE to redirect the stderr output, where FILE can be /dev/null, too. Thanks to Patrick Brunschwig for this idea!
  • Or, the --status-file FILE option could be used to direct the status lines to a temporary file.

Using --no-verbose and --status-file would be a fine solution!

@BjarniRunar

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

I'm just quite grateful we were included in the review! Testing all those different mail clients is lots of work. So thanks (again) for that.

I have working code using the --status-file approach in my local tree, I need to test it a bit more before I push and tag a new release candidate. I'd rather not send anything to /dev/null or reduce verbosity, as capturing such output can be critical during debugging. But mixing the streams will stop.

@BjarniRunar

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

Note: This fix is not super well tested. I am pretty confident it addresses the root problem, but it would still be nice if it were better tested.

I'm soliciting help with that here, we'll see how that goes: https://community.mailpile.is/t/johnny-you-are-fired-gnupg-bug-fix/143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.