Numeric truncation when parsing TYPEXX and CLASSXX representation

[headshog]

Hi! We've been fuzzing unbound with sydr-fuzz security predicates and we found numeric truncation error in str2wire.c:1978 and str2wire.c:2023.

In function sldns_str2wire_nsec_buf on line 2022 variable t has type uint16_t, function sldns_get_rr_type_by_name returns enum type sldns_rr_type that have unsigned int type value. The same is in sldns_str2wire_type_buf on line 2067. This enum has values that >= 65535. Out tool has found input where this function returns atoi(name + 4) and its value is bigger than 65535, so the numeric truncation occurs. So we suggest to add an assert checker in if operator in function sldns_get_rr_type_by_name on sldns/rrdef.c:704:

if (strlen(name) > 4 && strncasecmp(name, "TYPE", 4) == 0) {
    unsigned int a = atoi(name + 4);
    assert(a <= LDNS_RR_TYPE_LAST);
    return a;
}

Environment

• OS: ubuntu 20.04
• commit: 7240ecb

How to reproduce this error

1. Build docker container:

   sudo docker build -t oss-sydr-fuzz-unbound .
   
   

2. Run docker container:

   sudo docker run --privileged --network host -v /etc/localtime:/etc/localtime:ro --rm -it -v $PWD:/fuzz oss-sydr-fuzz-unboud /bin/bash
   
   

3. Run on the following input:

    /unbound_fuzzers/fuzz_3_fuzzer sydr_d1ceff6d6d77ddd69d8ec7b5b6e787ca4ca3ac97_num_trunc_0_signed.txt
   
   

4. Output:

   sldns/str2wire.c:2022:16: runtime error: implicit conversion from type 'sldns_rr_type' (aka 'enum sldns_enum_rr_type') of value 4294189519 (32-bit, unsigned) to type 'uint16_t' (aka 'unsigned short') changed the value to 8655 (16-bit, unsigned)
   SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior sldns/str2wire.c:2022:16
   
   sldns/str2wire.c:2067:15: runtime error: implicit conversion from type 'sldns_rr_type' (aka 'enum sldns_enum_rr_type') of value 4294189519 (32-bit, unsigned) to type 'uint16_t' (aka 'unsigned short') changed the value to 8655 (16-bit, unsigned)
   SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior sldns/str2wire.c:2067:15
   

[kobrineli]

@wcawijngaards
Hi! Could you take a look at these changes?

[edmonds]

The assert() may or may not run depending on whether the program is compiled with NDEBUG defined or not, but more importantly Unbound should not crash with an assertion in response to user input.

A better fix would be to make this function return an explicit failure code if the input isn't valid and update the callers of it to handle invalid input.

This function is originally from the ldns library (https://github.com/NLnetLabs/ldns/blob/67b9f0ae98910df882ca77f503673929a9e1db9b/rr.c#L2689-L2727) and in a library you would also not want to assert() if a caller provided unparseable input.

[headshog]

The assert() may or may not run depending on whether the program is compiled with NDEBUG defined or not, but more importantly Unbound should not crash with an assertion in response to user input.

A better fix would be to make this function return an explicit failure code if the input isn't valid and update the callers of it to handle invalid input.

This function is originally from the ldns library (https://github.com/NLnetLabs/ldns/blob/67b9f0ae98910df882ca77f503673929a9e1db9b/rr.c#L2689-L2727) and in a library you would also not want to assert() if a caller provided unparseable input.

@edmonds
I have added new enum LDNS_RR_TYPE_ERROR = LDNS_RR_TYPE_LAST - 1 in rrdefs.h and parsed case of this enum in functions where sldns_get_rr_type_by_name is called (except testcode/dohclient.c).

[edmonds]

	LDNS_RR_TYPE_LAST  = 65535,
	LDNS_RR_TYPE_ERROR = LDNS_RR_TYPE_LAST - 1,

OK so LDNS_RR_TYPE_ERROR is the numeric value 65534, but that value is a valid DNS RRTYPE, it's TYPE65534.

https://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#dns-parameters-4

[gthess]

Hi headshog, thanks for putting in the effort! By looking at the existing code I would return (enum sldns_enum_rr_type)0. Calling functions already check for something like (*tp == 0 && strcmp(token, "TYPE0") != 0) as an error condition. Also the same exact issue is present in the sldns_get_rr_class_by_name() function for the CLASSxx representation.

[headshog]

Hi headshog, thanks for putting in the effort! By looking at the existing code I would return (enum sldns_enum_rr_type)0. Calling functions already check for something like (*tp == 0 && strcmp(token, "TYPE0") != 0) as an error condition. Also the same exact issue is present in the sldns_get_rr_class_by_name() function for the CLASSxx representation.

@gthess Hi! I've applied changes that you requested.

[headshog]

@edmonds
I have mentioned @gthess, but didn't get any feedback( Could you please look at his suggestions and my new fixes?

[gthess]

Looks good apart from the return type used for sldns_get_rr_class_by_name().
It would be great if you also implement the (*tp == 0 && strcmp(token, "TYPE0") != 0) type of check when these are called, like you did in one of your previous commits :)

[headshog]

Looks good apart from the return type used for sldns_get_rr_class_by_name(). It would be great if you also implement the (*tp == 0 && strcmp(token, "TYPE0") != 0) type of check when these are called, like you did in one of your previous commits :)

Done.

[gthess]

I'll merge it the coming days so it will be part of the next release, thanks again!