I believe there has been a bug in KRL signature verification that has been present since the KRL feature was first introduced. It prevents signed KRLs from being loaded by OpenSSH [0]. I believe this bug applies to all versions of OpenSSH, although the majority of my effort has been devoted to (and all of my code snippets come from) openssl-portable. The bug is that an offset is incorrectly treated as a length [2]: /* Check signature over entire KRL up to this point */> if ((r = sshkey_verify(key, blob, blen, > sshbuf_ptr(buf), sshbuf_len(buf) - sig_off, 0)) != 0) > goto out;"sshbuf_len(buf) - sig_off" should read "sig_off". The result of this bug is that the number of unparsed bytes after our current parse cursor, rather than the number of parsed bytes before the cursor, is used as the length of the data to be verified. I don't believe this bug has any security implications, though, since both lengths are necessarily smaller than the length of buf. Fixing this bug uncovers another bug in ssh_krl_from_blob [3]: "if (sshbuf_len(sect) > 0)" should read "if (sect != NULL && sshbuf_len(sect) > 0)" (or similar), since a KRL_SECTION_SIGNATURE above might cause sect to be set to NULL. This bug results in a segmentation fault, but I don't believe it can be triggered without first fixing the above bug. In case anyone is interested in testing this behavior out, I believe the following hex-encoded string to be a spec-compliant [1] signed KRL: 5353484b524c0a00000000010000000043b9a3550000000043b9a3550000000000000000000000000000000001000000ac00000097000000077373682d727361000000030100010000008100aa28507872c5898cc95419d513f42a4b705fa0a212c5c8684d520375d88c5cb0d29cedb572cba9b07475e96e363511e7b0fb585ee9a3cc58db411abc3cd6e7c26d7d042a2ce842bd7af40522798680b9f1a54fdb598a80a170153995c8e3e63d411af5d436fdf2b9675c2e4bb3cc235a03e7f1955386aa5211ba919ecfb4137700000000200000000800000000000024520400000097000000077373682d727361000000030100010000008100aa28507872c5898cc95419d513f42a4b705fa0a212c5c8684d520375d88c5cb0d29cedb572cba9b07475e96e363511e7b0fb585ee9a3cc58db411abc3cd6e7c26d7d042a2ce842bd7af40522798680b9f1a54fdb598a80a170153995c8e3e63d411af5d436fdf2b9675c2e4bb3cc235a03e7f1955386aa5211ba919ecfb413770000008f000000077373682d727361000000801f3e023a97e606f90085bf09c11bc97ac1ef5ae2a8838d294c182f4d5201c3c9ed30d50c7f010a3f3b7aafb01d4b41b3b7afc33769638841c191d6661be995b310db6d4b90f4291a8a69d079d95bd6e9687ae96b4be58154a13f56680439ce4165170e219168db0ade9f4623b674dc4ff99498388b8344628d9662be3b4c75c0 It revokes a single certificate with serial 9298 issued by the following authority: ssh-rsa> AAAAB3NzaC1yc2EAAAADAQABAAAAgQCqKFB4csWJjMlUGdUT9CpLcF+gohLFyGhNUgN12IxcsNKc7bVyy6mwdHXpbjY1Eeew+1he6aPMWNtBGrw81ufCbX0EKizoQr169AUieYaAufGlT9tZioChcBU5lcjj5j1BGvXUNv3yuWdcLkuzzCNaA+fxlVOGqlIRupGez7QTdw=The KRL also has a single signature from this authority. Carl [0]: OpenSSH itself never produces signed KRLs, and so far as I've been able to tell nobody else does either. I found this while adding KRL support based on the spec [1] to Go (golang)'s x/crypto/ssh package. [1]: https://github.com/openssh/openssh-portable/blob/271df8185d9689b3fb0523f58514481b858f6843/PROTOCOL.krl [2]: https://github.com/openssh/openssh-portable/blob/271df8185d9689b3fb0523f58514481b858f6843/krl.c#L1010-L1019 [3]: https://github.com/openssh/openssh-portable/blob/271df8185d9689b3fb0523f58514481b858f6843/krl.c#L1095-L1104
There seems to be at least one more bug with KRL signature verification: the first pass loop that gathers signatures terminates after the first signature [0]. I think removing the "break" at the end of the loop is sufficient to fix this behavior for KRLs signed multiple times. Carl [0]: https://github.com/openssh/openssh-portable/blob/271df8185d9689b3fb0523f58514481b858f6843/krl.c#L1039 On Tue, Dec 29, 2015 at 6:23 PM, Carl Jackson <carl at stripe.com> wrote:> I believe there has been a bug in KRL signature verification that has been > present since the KRL feature was first introduced. It prevents signed KRLs > from being loaded by OpenSSH [0]. I believe this bug applies to all > versions of OpenSSH, although the majority of my effort has been devoted to > (and all of my code snippets come from) openssl-portable. > > The bug is that an offset is incorrectly treated as a length [2]: > > /* Check signature over entire KRL up to this point */ >> if ((r = sshkey_verify(key, blob, blen, >> sshbuf_ptr(buf), sshbuf_len(buf) - sig_off, 0)) != 0) >> goto out; > > > "sshbuf_len(buf) - sig_off" should read "sig_off". The result of this bug > is that the number of unparsed bytes after our current parse cursor, rather > than the number of parsed bytes before the cursor, is used as the length of > the data to be verified. I don't believe this bug has any security > implications, though, since both lengths are necessarily smaller than the > length of buf. > > Fixing this bug uncovers another bug in ssh_krl_from_blob [3]: "if > (sshbuf_len(sect) > 0)" should read "if (sect != NULL && sshbuf_len(sect) > > 0)" (or similar), since a KRL_SECTION_SIGNATURE above might cause sect to > be set to NULL. This bug results in a segmentation fault, but I don't > believe it can be triggered without first fixing the above bug. > > In case anyone is interested in testing this behavior out, I believe the > following hex-encoded string to be a spec-compliant [1] signed KRL: > > >> 5353484b524c0a00000000010000000043b9a3550000000043b9a3550000000000000000000000000000000001000000ac00000097000000077373682d727361000000030100010000008100aa28507872c5898cc95419d513f42a4b705fa0a212c5c8684d520375d88c5cb0d29cedb572cba9b07475e96e363511e7b0fb585ee9a3cc58db411abc3cd6e7c26d7d042a2ce842bd7af40522798680b9f1a54fdb598a80a170153995c8e3e63d411af5d436fdf2b9675c2e4bb3cc235a03e7f1955386aa5211ba919ecfb4137700000000200000000800000000000024520400000097000000077373682d727361000000030100010000008100aa28507872c5898cc95419d513f42a4b705fa0a212c5c8684d520375d88c5cb0d29cedb572cba9b07475e96e363511e7b0fb585ee9a3cc58db411abc3cd6e7c26d7d042a2ce842bd7af40522798680b9f1a54fdb598a80a170153995c8e3e63d411af5d436fdf2b9675c2e4bb3cc235a03e7f1955386aa5211ba919ecfb413770000008f000000077373682d727361000000801f3e023a97e606f90085bf09c11bc97ac1ef5ae2a8838d294c182f4d5201c3c9ed30d50c7f010a3f3b7aafb01d4b41b3b7afc33769638841c191d6661be995b310db6d4b90f4291a8a69d079d95bd6e9687ae96b4be58154a13f56680439ce4165170e219168db0ade9f4623b674dc4ff99498388b8344628d9662be3b4c75c0 > > > It revokes a single certificate with serial 9298 issued by the following > authority: > > ssh-rsa >> AAAAB3NzaC1yc2EAAAADAQABAAAAgQCqKFB4csWJjMlUGdUT9CpLcF+gohLFyGhNUgN12IxcsNKc7bVyy6mwdHXpbjY1Eeew+1he6aPMWNtBGrw81ufCbX0EKizoQr169AUieYaAufGlT9tZioChcBU5lcjj5j1BGvXUNv3yuWdcLkuzzCNaA+fxlVOGqlIRupGez7QTdw=> > > The KRL also has a single signature from this authority. > > Carl > > [0]: OpenSSH itself never produces signed KRLs, and so far as I've been > able to tell nobody else does either. I found this while adding KRL support > based on the spec [1] to Go (golang)'s x/crypto/ssh package. > [1]: > https://github.com/openssh/openssh-portable/blob/271df8185d9689b3fb0523f58514481b858f6843/PROTOCOL.krl > [2]: > https://github.com/openssh/openssh-portable/blob/271df8185d9689b3fb0523f58514481b858f6843/krl.c#L1010-L1019 > [3]: > https://github.com/openssh/openssh-portable/blob/271df8185d9689b3fb0523f58514481b858f6843/krl.c#L1095-L1104 >
On Tue, 29 Dec 2015, Carl Jackson wrote:> There seems to be at least one more bug with KRL signature verification: > the first pass loop that gathers signatures terminates after the first > signature [0]. I think removing the "break" at the end of the loop is > sufficient to fix this behavior for KRLs signed multiple times.Thanks for the detailed reports. Here are the fixes that I made - I'll commit them shortly. Please let me know if you end up making any more test KRLs for your work on the Go implementation, I might add them to our test suite. -d Index: krl.c ==================================================================RCS file: /cvs/src/usr.bin/ssh/krl.c,v retrieving revision 1.36 diff -u -p -r1.36 krl.c --- krl.c 11 Dec 2015 04:21:12 -0000 1.36 +++ krl.c 30 Dec 2015 23:41:06 -0000 @@ -1013,7 +1013,7 @@ ssh_krl_from_blob(struct sshbuf *buf, st } /* Check signature over entire KRL up to this point */ if ((r = sshkey_verify(key, blob, blen, - sshbuf_ptr(buf), sshbuf_len(buf) - sig_off, 0)) != 0) + sshbuf_ptr(buf), sig_off, 0)) != 0) goto out; /* Check if this key has already signed this KRL */ for (i = 0; i < nca_used; i++) { @@ -1034,7 +1034,6 @@ ssh_krl_from_blob(struct sshbuf *buf, st ca_used = tmp_ca_used; ca_used[nca_used++] = key; key = NULL; - break; } if (sshbuf_len(copy) != 0) { @@ -1099,7 +1098,7 @@ ssh_krl_from_blob(struct sshbuf *buf, st r = SSH_ERR_INVALID_FORMAT; goto out; } - if (sshbuf_len(sect) > 0) { + if (sect != NULL && sshbuf_len(sect) > 0) { error("KRL section contains unparsed data"); r = SSH_ERR_INVALID_FORMAT; goto out;