openssh bugs - Jul 2020 - [Bug 3198] New: Custom critical options are not lexically ordered

https://bugzilla.mindrot.org/show_bug.cgi?id=3198

            Bug ID: 3198
           Summary: Custom critical options are not lexically ordered
           Product: Portable OpenSSH
           Version: -current
          Hardware: amd64
                OS: Mac OS X
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh-keygen
          Assignee: unassigned-bugs at mindrot.org
          Reporter: mariano.cano at gmail.com

According to PROTOCOL.certkeys critical options must be lexically
ordered by name:

```
Options must be lexically ordered by "name" if they appear in the
sequence. Each named option may only appear once in a certificate.
```

This works for the well-known options 'source-address' and
'force-command' but if custom critical options are passed, they will
appear in the order passed in:

```
$ ssh-keygen -I key-id -O critical:a at foo.com=a -O critical:c at foo.com=c
-O critical:b at foo.com=b -s ca.key user.pub
Signed user key user-cert.pub: id "key-id" serial 0 valid forever
```

They will be stored in the same order and will appear in that order
when inspecting the certificate:
```
user-cert.pub:
        Type: ssh-rsa-cert-v01 at openssh.com user certificate
        Public key: RSA-CERT
SHA256:5XSDsn5usZ40pRyDsJnR2KWKacRc29ufR+U8KLYBqGw
        Signing CA: ECDSA
SHA256:rmAjupXaId7QQode/ThbhY/t427k9EKtTfNQHn5AkPk (using
ecdsa-sha2-nistp256)
        Key ID: "key-id"
        Serial: 0
        Valid: forever
        Principals: (none)
        Critical Options:
                a at foo.com UNKNOWN OPTION (len 5)
                c at foo.com UNKNOWN OPTION (len 5)
                b at foo.com UNKNOWN OPTION (len 5)
        Extensions:
                permit-X11-forwarding
                permit-agent-forwarding
                permit-port-forwarding
                permit-pty
                permit-user-rc
```

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3198

--- Comment #1 from Mariano Cano <mariano.cano at gmail.com> ---
The same will occur on regular extension, PROTOCOL.certkeys states:

... The encoding and ordering of extensions in this field is 
identical to that of the critical options, as is the requirement
that each name appear only once. 

But a certificate created with the following command will have the
specified extension at the end of the standard ones:

ssh-keygen -s ca-key -I key-id -O extension:login at github.com=username
user-key.pub

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3198

Mariano Cano <mariano.cano at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Custom critical options are |Custom critical options and
                   |not lexically ordered       |extensions are not
                   |                            |lexically ordered

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3198

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
                 CC|                            |djm at mindrot.org,
                   |                            |dtucker at dtucker.net
   Attachment #3434|                            |ok?(dtucker at dtucker.net)
              Flags|                            |

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
Created attachment 3434
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3434&action=edit
explicitly sort certificate extensions sections

Nice catch. This should fix it.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3198

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |3162


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=3162
[Bug 3162] Tracking bug for 8.4 release
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3198

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3434|ok?(dtucker at dtucker.net)    |ok+
              Flags|                            |

--- Comment #3 from Darren Tucker <dtucker at dtucker.net> ---
Comment on attachment 3434
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3434
explicitly sort certificate extensions sections

assuming the tests pass :-)

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3198

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
patch applied - thanks

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3198

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #5 from Darren Tucker <dtucker at dtucker.net> ---
Mass close of all bugs fixed in 8.4 release.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.