APT does not properly handle expired or revoked key signatures

I looked at the branch that Michael posted and this is a no-go for a security update for stable because:

a) it adds new strings
b) it changes config option names:
- string pubringpath = _config->Find("APT::GPGV::TrustedKeyring", "/etc/apt/trusted.gpg");
+ string pubringpath = _config->Find("APT::gpg::TrustedKeyring", "/etc/apt/trusted.gpg");

Currently I believe this is a problem with gpgv and should adressed there (also there is some argument
about this given that the man page for gpgv states that it will trust any key).

So it seems like the check for VALIDSIG is incorrect (also the doc/DETAILS says:
   VALIDSIG <fingerprint in hex> <sig_creation_date> <sig-timestamp>
                <expire-timestamp> <sig-version> <reserved> <pubkey-algo>
                <hash-algo> <sig-class> <primary-key-fpr>

        The signature with the keyid is good. This is the same as
        GOODSIG but has the fingerprint as the argument.
)

The doc for GOOGSIG says:
   GOODSIG <long keyid> <username>
        The signature with the keyid is good. For each signature only
        one of the three codes GOODSIG, BADSIG or ERRSIG will be
        emitted and they may be used as a marker for a new signature.

So it seems validsig is written on the status-fd but no goodsig for revoked/expired keys, here is what I see (I ran it with both gpgv and gpg to be able to compare the output):

Here is what gpgv/gpg outputs (on the status fd):

[valid]
$ gpgv --status-fd 1 --keyring /etc/apt/trusted.gpg /var/lib/apt/lists/archive.canonical.com_ubuntu_dists_jaunty_Release.gpg /var/lib/apt/lists/archive.canonical.com_ubuntu_dists_jaunty_Release
gpgv: Signature made 2009-01-21T20:40:58 CET using DSA key ID 437D05B5
[GNUPG:] SIG_ID yBuoChDxaZ0UKcDy7tLCPl5qQ2M 2009-01-21 1232566858
[GNUPG:] GOODSIG 40976EAF437D05B5 Ubuntu Archive Automatic Signing Key <email address hidden>
gpgv: Good signature from "Ubuntu Archive Automatic Signing Key <email address hidden>"
[GNUPG:] VALIDSIG 630239CC130E1A7FD81A27B140976EAF437D05B5 2009-01-21 1232566858 0 3 0 17 2 00 630239CC130E1A7FD81A27B140976EAF437D05B5

$ gpg --verify --status-fd 1 --keyring /etc/apt/trusted.gpg /var/lib/apt/lists/archive.canonical.com_ubuntu_dists_jaunty_Release.gpg /var/lib/apt/lists/archive.canonical.com_ubuntu_dists_jaunty_Release
gpg: WARNING: unsafe ownership on configuration file `/home/egon/.gnupg/gpg.conf'
gpg: Signature made 2009-01-21T20:40:58 CET using DSA key ID 437D05B5
[GNUPG:] SIG_ID yBuoChDxaZ0UKcDy7tLCPl5qQ2M 2009-01-21 1232566858
[GNUPG:] GOODSIG 40976EAF437D05B5 Ubuntu Archive Automatic Signing Key <email address hidden>
gpg: Good signature from "Ubuntu Archive Automatic Signing Key <email address hidden>"
[GNUPG:] VALIDSIG 630239CC130E1A7FD81A27B140976EAF437D05B5 2009-01-21 1232566858 0 3 0 17 2 00 630239CC130E1A7FD81A27B140976EAF437D05B5
[GNUPG:] TRUST_UNDEFINED
gpg: WARNING: This key is not certified with a trusted signature!
gpg: There is no indication that the signature belongs to the owner.
Primary key fingerprint: 6302 39CC 130E 1A7F D81A 27B1 4097 6EAF 437D 05B5

[expired]
$ gpgv --status-fd 1 expired/lala.sig expired/lala
gpgv: Signature made Wed Apr 1 19:40:00 2009 UTC using DSA key ID 89F9D59A
[GNUPG:] KEYEXPIRED 1238701001
[GNUPG:] SIGEXPIRED deprecated-use-keyexpired-instead
[GNUPG:] KEYEXPIRED 1238701001
[GNUPG:] SIGEXPIRED deprecated-use-keyexpired-instead
[GNUPG:] SIG_ID Kzi0hzrQmTz3f+1M7m1VKTMAyeA 2009-04-01 1238614800
[GNUPG:] KEYEXPIRED 1238701001
[GNUPG:] SIGEXPIRED deprecated-use-keyexpired-instead
[GNUPG:] EXPKEYSIG 17427FB989F9D59A foo bar baz <email address hidden>
gpgv: Good signature from "foo bar baz <email address hidden>"
[GNUPG:] VALIDSIG 526A66FC781E11945CC532A817427FB989F9D59A 2009-04-01 1238614800 0 4 0 17 2 00 526A66FC781E11945CC532A817427FB989F9D59A

$ gpg --status-fd 1 --verify expired/lala.sig expired/lala
gpg: Signature made Wed Apr 1 19:40:00 2009 UTC using DSA key ID 89F9D59A
[GNUPG:] KEYEXPIRED 1238701001
[GNUPG:] SIGEXPIRED deprecated-use-keyexpired-instead
[GNUPG:] KEYEXPIRED 1238701001
[GNUPG:] SIGEXPIRED deprecated-use-keyexpired-instead
[GNUPG:] SIG_ID Kzi0hzrQmTz3f+1M7m1VKTMAyeA 2009-04-01 1238614800
[GNUPG:] KEYEXPIRED 1238701001
[GNUPG:] SIGEXPIRED deprecated-use-keyexpired-instead
[GNUPG:] EXPKEYSIG 17427FB989F9D59A foo bar baz <email address hidden>
gpg: Good signature from "foo bar baz <email address hidden>"
[GNUPG:] VALIDSIG 526A66FC781E11945CC532A81742...

Does this affect all releases?

My reading of the above is that the gpg method should check for GOODSIG and only accept signatures that have that, VALIDSIG is not enough. This looks like it might be trick to do without adding new strings :/

It also needs to be verified that all versions of gpgv (back down to dapper) behave consistently in the way that is shown above. The test above was done using jaunty.

I just checked if the behavior is the same on dapper and my tests indicate it is. Additional testing/clarification is very welcome.

(home in the crude test environement is HOME=/home/egon - sorry for that).

I looked at the code in gpg/gpgv at mainproc.c:check_sig_and_print() and GOODSIG is only set if its neither bad/expired/revoked:
...
 if(rc)
   statno=STATUS_BADSIG;
 else if(sig->flags.expired)
   statno=STATUS_EXPSIG;
 else if(is_expkey)
   statno=STATUS_EXPKEYSIG;
 else if(is_revkey)
   statno=STATUS_REVKEYSIG;
 else
   statno=STATUS_GOODSIG;
...

Validsig OTOH is returned ignoring the above statuses.

Here is a proposed fix. It does the following:

 * recognize KEYEXPIRED and KEYREVOKED messages from gpgv and put them into a new "WorthlessSignatures " vector
 * only listen for GOODSIG message from gpgv and ignore VALIDSIG (as GOODSIG is only send when the signature is not with a expired or revoked key)
 * if there is no good signature, show a message that displayes the worthless signatures to the user (including the KEYEXPIRED or KEYREVSIG bits to ensure there is a way to know what is going on)
 * if there is one (or more) good signature and worthless signatures, just ignore the worthless ones

That should hopefully cover the problem without breaking strings and compatibility. Feedback/review/testing is very welcome. I tested it in a etch chroot with various expired settings and it works as it should, but I need to make a test-suit for it too. I will also pass it for review to debian.

 I did a simple "test-suite" to test problem and
fix at:
$ bzr get lp:~mvo/+junk/apt-gpgv-tests
$ cd apt-gpgv-tests
$ sudo ./test.sh

It does test "good","expired","revoked","revoked+good","expired+good".

It should fail for expired and revoked (they are no good) but should
pass for revoked+good and expired+good. Basicly it will ignore expired
or revoked signatures if there is anohter valid signature on the
Release file.

Again, feedback/review is very welcome.

Cheers,
 Michael

Michael, I'll go through this first thing tomorrow.

I've confirmed that Dapper - Jaunty are all affected by the bug and have consistent GOODSIG behavior.

I spent quite a bit of time reviewing this and feel the the patch is correct. It applies cleanly to intrepid and hardy, has some fuzz on gutsy and needed small adjustments for dapper. I also verified the strings for dapper-intrepid, and there are no new string changes.

I went through the logic of the VerifyGetSigners by hand using desk-checking (see attached for review) for GoodSigners, BadSigners, WorthlessSigners, and NoPubKeySigners being either empty or not empty, and believe the patch to be correct.

I also added several tests to Michael's test suite to ensure apt is working properly after patching and did not introduce regressions. I'll make these available shortly.

I added some extra tests to https://code.launchpad.net/~jdstrand/+junk/apt-gpgv-tests. To use:

$ bzr get lp:~jdstrand/+junk/apt-gpgv-tests
$ cd apt-gpgv-tests
$ sudo ./test.sh

Michael, I plan to add these to https://launchpad.net/~ubuntu-bugcontrol/qa-regression-testing/master/ which is why there is copyright information in my branch. Please let me know if I should change anything or you don't want it included.

Thanks a lot Jamie for the additional tests and the desk_check.txt. That is great.

The copyright is fine, I was just to lazy^Wbusy to add it myself. Please add it to the tracker, I will do the jaunty upload in parallel (including your fix for the date problem).

This bug was fixed in the package apt - 0.7.20.2ubuntu6

---------------
apt (0.7.20.2ubuntu6) jaunty; urgency=low

  [ Jamie Strandboge ]
  * apt.cron.daily: catch invalid dates due to DST time changes
    in the stamp files (LP: #354793)

  [ Michael Vogt ]
  * methods/gpgv.cc:
    - properly check for expired and revoked keys (closes: #433091)
      LP: #356012

 -- Michael Vogt <email address hidden> Wed, 08 Apr 2009 22:39:50 +0200

I'm won't have an upload for Gutsy, because I won't be publishing this until next week, and Gutsy will be EOL.

FYI-- the added apt tests have been added to qa-regression-testing in http://bazaar.launchpad.net/~ubuntu-bugcontrol/qa-regression-testing/master/files/head%3A/scripts/apt/ and the previous 'junk' url removed.

http://www.ubuntu.com/usn/usn-762-1