Bug 14238 - asn1 problem found in oss-fuzz
Summary: asn1 problem found in oss-fuzz
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
Depends on:
Reported: 2020-01-16 22:30 UTC by Douglas Bagnall
Modified: 2020-01-30 03:04 UTC (History)
0 users

See Also:

Possible patch for master. (1.47 KB, patch)
2020-01-23 22:13 UTC, Jeremy Allison
dbagnall: review+
jra: ci-passed+

Note You need to log in before you can comment on or make changes to this bug.
Description Douglas Bagnall 2020-01-16 22:30:46 UTC

Jeremy: I have noticed that you have a strong emotional response to certain combinations of the characters 'A', 'S', 'N', and '1'.

I am sorry to say that means you spring to mind when I see that word, and I assume you have LOTS OF VALUABLE EXPERIENCE.

The fuzzer is complaining about this function:

/* read an integer */                                                                                                          
bool asn1_read_enumerated(struct asn1_data *data, int *v)                                                                      
        *v = 0;                                                                                                                
        if (!asn1_start_tag(data, ASN1_ENUMERATED)) return false;                                                              
        while (!data->has_error && asn1_tag_remaining(data)>0) {                                                               
                uint8_t b;                                                                                                     
                if (!asn1_read_uint8(data, &b)) {                                                                              
                        return false;                                                                                          
                *v = (*v << 8) + b;                                                                                            
        return asn1_end_tag(data);                                                                                             

because *v builds up to be greater than 1<<24, then on the next round it is not going to fit in 32 bits. It calls this undefined, which is technically true I guess, though in practice it is always going to chuck out some significant bits and possibly flip the sign. But that still doesn't really seem good. And if we limit it sizeof(int) rounds, it can still flip the sign, which is probably undefined anyway. It is lucky that sizeof(int) is de-facto fixed, otherwise we'd get different decodings on different architectures.

What is the right thing to do here?
Comment 1 Jeremy Allison 2020-01-23 20:21:57 UTC
I'll try and take a look soon. Thanks.
Comment 2 Jeremy Allison 2020-01-23 20:23:55 UTC
Here is what libssl does here:



ASN1_ENUMERATED_get() returns the value of a in a similar way to ASN1_INTEGER_get() but it returns 0xffffffffL if the value of a will not fit in a long type. New applications should use ASN1_ENUMERATED_get_int64() instead.
Comment 3 Jeremy Allison 2020-01-23 20:24:16 UTC
We should probably do the same I think.
Comment 4 Jeremy Allison 2020-01-23 20:55:41 UTC
OK, I think an ASN.1 ENUMERATED type has to be non-negative. Still investigating.
Comment 5 Jeremy Allison 2020-01-23 21:21:17 UTC



Enumerated Values

"Enumerated elements should not have negative numeric values. However, values are still encoded using the two’s complement representation of the value, so that it may be necessary to add a leading byte in which all bits are set to zero if the binary representation of the numeric value would have otherwise caused the most significant bit in the first byte to be set to one."
Comment 6 Jeremy Allison 2020-01-23 22:13:42 UTC
Created attachment 15751 [details]
Possible patch for master.

Here's what I've got in CI right now. Let me know what you think !
Comment 7 Jeremy Allison 2020-01-23 23:29:53 UTC
Comment on attachment 15751 [details]
Possible patch for master.

Passes CI. Let me know if you want a merge request !
Comment 8 Jeremy Allison 2020-01-28 23:12:37 UTC
Ping - do you think this one works ?
Comment 9 Douglas Bagnall 2020-01-28 23:16:16 UTC
(In reply to Jeremy Allison from comment #8)
Yes, looks good. RB+!
Comment 10 Douglas Bagnall 2020-01-29 01:38:43 UTC
Is this worth backporting?
Comment 11 Douglas Bagnall 2020-01-30 03:04:49 UTC
Oh, we should have added "credit to OSS-Fuzz".