Bug 3269

Summary: sshbuf_get_u32() called with enum type argument in monitor.c
Product: Portable OpenSSH Reporter: Stephen Goetze <steve>
Component: sshdAssignee: Assigned to nobody <unassigned-bugs>
Status: CLOSED FIXED    
Severity: minor CC: balu.gajjala, djm, dtucker
Priority: P5    
Version: 8.4p1   
Hardware: Other   
OS: All   
Bug Depends on:    
Bug Blocks: 3270    
Attachments:
Description Flags
explicitly use uint32 for enum in monitor none

Description Stephen Goetze 2021-03-04 04:45:47 AEDT 
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.
Comment 1 Darren Tucker 2021-03-04 08:28:57 AEDT 
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?
Comment 2 Darren Tucker 2021-03-04 08:30:34 AEDT 
(there's a separate question of correctness on a platform where int != 32bits, though)
Comment 3 Stephen Goetze 2021-03-04 09:43:26 AEDT 
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.
Comment 4 Damien Miller 2021-03-04 09:57:11 AEDT 
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
Comment 5 Darren Tucker 2021-03-04 10:00:20 AEDT 
Created attachment 3475 [details]
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?
Comment 6 Stephen Goetze 2021-03-05 00:32:08 AEDT 
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
Comment 7 Darren Tucker 2021-04-09 14:12:03 AEST 
*** Bug 3294 has been marked as a duplicate of this bug. ***
Comment 8 Damien Miller 2021-04-23 15:03:36 AEST 
closing resolved bugs as of 8.6p1 release