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

gnupg: add patch disallowing compressed signatures and certificates #180336

Merged

Conversation

stigtsp
Copy link
Member

@stigtsp stigtsp commented Jul 6, 2022

Description of changes

Adds a patch by Demi Marie Obenour that disallows compressed signatures and certificates to prevent DoS attacks.

https://seclists.org/oss-sec/2022/q3/9
https://seclists.org/oss-sec/2022/q3/27

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@vcunat
Copy link
Member

vcunat commented Jul 7, 2022

This patch broke tests of gpgme and thus many builds.

EDIT: the log is a little messy, so let me post the relevant part:

t-verify:310: Double plaintext message not detected
FAIL: t-verify

@vcunat vcunat mentioned this pull request Jul 7, 2022
@stigtsp
Copy link
Member Author

stigtsp commented Jul 7, 2022

This patch broke tests of gpgme and thus many builds.

Ok, would need to look into the test and see if it can be skipped. Unfortunately, I can't work on this today :-/

@vcunat
Copy link
Member

vcunat commented Jul 7, 2022

So, it seems very likely that it's the test's fault, not an oversight in the patch?

@stigtsp
Copy link
Member Author

stigtsp commented Jul 7, 2022

Yes, i believe so. Have confirmed that the patch fixes the problem described by the author, i.e. rejecting the problematic signature.

vcunat added a commit that referenced this pull request Jul 7, 2022
/cc PR #180336

I'm not entirely sure about this, as I couldn't spend much time, but
it seemed plausible that the patch caused a different kind of errors
in this tested case - though it's possible I messed the test up.
Either way, the tests seem to pass now, unblocking the CVE fixes ;-)
@vcunat
Copy link
Member

vcunat commented Jul 7, 2022

OK, for now: db6b3e0

@vcunat
Copy link
Member

vcunat commented Jul 7, 2022

Eww, with 1fc7604

@stigtsp
Copy link
Member Author

stigtsp commented Jul 7, 2022

Hi @DemiMarie! Thx for posting about the recent issues with GnuPG on oss-sec.

Just CCing you on this issue as the patch seems to break tests in gpgme, in case you have any advice if 1fc7604 is reasonable.

@DemiMarie
Copy link

Hi @DemiMarie! Thx for posting about the recent issues with GnuPG on oss-sec.

You’re welcome! I should probably fix the cleartext signature case at some point too.

Just CCing you on this issue as the patch seems to break tests in gpgme, in case you have any advice if 1fc7604 is reasonable.

It does look reasonable, but for a security patch a better approach might be to use BAD_DATA instead of UNEXPECTED elsewhere in the code.

vcunat added a commit that referenced this pull request Jul 8, 2022
This is a python counterpart of commit db6b3e0; /cc PR #180336
stigtsp pushed a commit to stigtsp/nixpkgs that referenced this pull request Jul 8, 2022
/cc PR NixOS#180336

I'm not entirely sure about this, as I couldn't spend much time, but
it seemed plausible that the patch caused a different kind of errors
in this tested case - though it's possible I messed the test up.
Either way, the tests seem to pass now, unblocking the CVE fixes ;-)

(cherry picked from commit db6b3e0)
stigtsp pushed a commit to stigtsp/nixpkgs that referenced this pull request Jul 8, 2022
This is a python counterpart of commit db6b3e0; /cc PR NixOS#180336

(cherry picked from commit add0201)
@vcunat
Copy link
Member

vcunat commented Jul 13, 2022

  • Another gpgme test regressed on i686-linux (only, so probably not important):
********* Start testing of AddExistingSubkeyJobTest *********
Config: Using QtTest library 5.15.3, Qt 5.15.3 (i386-little_endian-ilp32 shared (dynamic) release build; by GCC 10.4.0), unknown unknown
PASS   : AddExistingSubkeyJobTest::initTestCase()
PASS   : AddExistingSubkeyJobTest::testAddExistingSubkeyAsync()
PASS   : AddExistingSubkeyJobTest::testAddExistingSubkeySync()
FAIL!  : AddExistingSubkeyJobTest::testAddExistingSubkeyWithExpiration() 'result.code() == GPG_ERR_NO_ERROR' returned FALSE. ()
   Loc: [t-addexistingsubkey.cpp(216)]
PASS   : AddExistingSubkeyJobTest::cleanupTestCase()
Totals: 4 passed, 1 failed, 0 skipped, 0 blacklisted, 2058ms
********* Finished testing of AddExistingSubkeyJobTest *********
QTemporaryDir: Unable to remove "/build/t-addexistingsubkey-jLOarG" most likely due to the presence of read-only files.
FAIL: t-addexistingsubkey

https://hydra.nixos.org/log/s58rncd0idgzh5pmk8f6myb83fj471ww-gpgme-1.17.1.drv

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants