openssh bugs - Mar 2021 - [Bug 3269] New: sshbuf_get_u32() called with enum type argument in monitor.c

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

            Bug ID: 3269
           Summary: sshbuf_get_u32() called with enum type argument in
                    monitor.c
           Product: Portable OpenSSH
           Version: 8.4p1
          Hardware: Other
                OS: All
            Status: NEW
          Severity: minor
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: goetze at dovetail.com

sshbuf_get_u32() as declared in sshbuf.h takes a u_int_t* as the
argument to set:

int sshbuf_get_u32(struct sshbuf *buf, u_int32_t *valp);

However, in monitor.c mm_answer_keyallowed() an enum type is passed
(from the 8.4p1 code base):

line 1156: enum mm_keytype type = 0;
line 1161: if ((r = sshbuf_get_u32(m, &type)) != 0 ||

This usage is not safe for implementations that size enum types based
on the smallest type that will fit the set of enum values.  My reading
of the C99 standard says that this "size only to fit" approach is an
option left to the implementation (from 6.7.2.2 Enumeration
specifiers):

"Each enumerated type shall be compatible with char, a signed integer
type, or an unsigned integer type. The choice of type is
implementation-defined,110) but shall be capable of representing the
values of all the members of the enumeration."

We are working with a compiler that implements this behavior, and it
does issue a warning about line 1161 above.

If the warning is ignored, the wrong value is in fact set in the enum
type.

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

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

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at dtucker.net

--- Comment #1 from Darren Tucker <dtucker at dtucker.net> ---
OpenSSH has traditionally required only c89, and that (or at least the
drafts of it available online) says:

6.1.3.3 Enumeration constants
[...]
   An identifier declared as an enumeration constant has type int.

configure calls AC_PROG_CC which automatically calls AC_PROG_CC_C89, so
the compiler should be in c89 mode.

What is the compiler in question?  Does it do c89?

-- 
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=3269

--- Comment #2 from Darren Tucker <dtucker at dtucker.net> ---
(there's a separate question of correctness on a platform where int !32bits,
though)

-- 
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=3269

--- Comment #3 from Stephen Goetze <goetze at dovetail.com> ---
The compiler is IBM xlc, and I had been building with c99 mode - the
INSTALL does say that any C89 or better compiler should work.

When I switch to c89, the problem persists, so it appears that IBM
doesn't downshift their treatment of enumerations when running in c89
mode.

IBM does have a #pragma enum(size) where the "size only to fit"
behavior can be overridden; that seems like the best option for us to
use.

You may want to make a note of this new behavior in c99 if you ever
decide to specify a newer standard for building; the potential exists
for a nasty bug.

Thanks for your quick response, you can close this.

-- 
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=3269

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |3270


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=3270
[Bug 3270] Tracking bug for 8.6 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=3269

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
                 CC|                            |djm at mindrot.org

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
Writing an overflowing value into an int is still undefined behaviour
so we want to avoid that too.

In any case, it's trivial to change the type of the received value to
u_int, so I committed that as 160db17fc678

-- 
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=3269

--- Comment #5 from Darren Tucker <dtucker at dtucker.net> ---
Created attachment 3475
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3475&action=edit
explicitly use uint32 for enum in monitor

(In reply to Stephen Goetze from comment #3)
> When I switch to c89, the problem persists, so it appears that IBM
> doesn't downshift their treatment of enumerations when running in
> c89 mode.
That sounds like a compiler bug.
> You may want to make a note of this new behavior in c99 if you ever
> decide to specify a newer standard for building; the potential
> exists for a nasty bug.
I think we should still do something about this.  We're slowly using
C99 features (eg recently, variadic macros) and I don't think it's
strictly correct for C89 either: the same problem could occur on a
platform where int<32bits (although I know of no such platform that
it'll currently run on) or int>32 bits (which is plausible, and I'd
imagine whether or not it would work properly would depend on the
endianness).

Does this patch fix it for you?

-- 
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=3269

--- Comment #6 from Stephen Goetze <goetze at dovetail.com> ---
Thanks for the patch.  I'm wondering if it isn't better in this case to
not ever introduce the enum type at all here.  How about just changing
the type of "type" to u_int32_t?

This way, if there is any garbage in the high order bytes in the
sshbuf, that won't be lost in the assignment conversion, and the switch
that follows would detect any bad data.  In the switch, the conversion
go the right direction, from enum to u_int32_t

-- 
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=3269

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |balu.gajjala at gmail.com

--- Comment #7 from Darren Tucker <dtucker at dtucker.net> ---
*** Bug 3294 has been marked as a duplicate of this bug. ***

-- 
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=3269

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #8 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 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.