Bug in KRL signature verification

[Carl Jackson]

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
>