[CVE-2018-6480] - type confusion in ccnl_fwd_handleInterest

[mfrey]

Hi,

the following code in ccnl_fwd_handleInterest assumes that the union member s is of type ccnl_pktdetail_ndntlv_s. However, if the type is in fact of type struct ccnl_pktdetail_ccntlv_s or struct ccnl_pktdetail_iottlv_s, the memory at that point is either uninitialised or points to data which is not a nonce, which renders the code using the local variable nonce pointless.

EDIT: There is a check for nonce != NULL in line 235, but I'm not sure if this is sufficient. If for example (*pkt)->s is of type ccnl_pktdetail_ccntlv_s it points to its sole member struct ccnl_buf_s *keyid which in turn might not be NULL. Nevertheless, the rest of the following lines in the function assume that the local variable is set to an actual ''nonce'' value.

235     if (pkt != NULL && (*pkt) != NULL && (*pkt)->s.ndntlv.nonce != NULL) {
236         if ((*pkt)->s.ndntlv.nonce->datalen == 4) {
237             memcpy(&nonce, (*pkt)->s.ndntlv.nonce->data, 4);
238         }
239     }

This goes on and on. The function in ccnl_nonce_isDup (guarded by a USE_DUP_CHECK) also starts to iterate over the pit and checks if the (maybe non-existing nonce field) matches data in the pit, i.e.

 897     if(CCNL_MAX_NONCES < 0){
 898         struct ccnl_interest_s *i = NULL;
 899         for (i = relay->pit; i; i = i->next) {
 900             if(buf_equal(i->pkt->s.ndntlv.nonce, pkt->s.ndntlv.nonce)){
 901                 return 1;
 902             }

If this is an issue how do we handle it? I would start writing checks and guards, but I want to make sure that you consider this an issue as well (and I'm not missing a point here).

TIA Michael

[mfrey]

Also, CCNL_MAX_NONCES seems to be introduced as a check if somebody is running this on RIOT or not. While I'm not a particular huge fan of this ifdef hell, this is hell of confusing.

 70 #ifdef CCNL_RIOT
 71 #define CCNL_MAX_NONCES                 -1 // -1 --> detect dups by PIT
 72 #else //!CCNL_RIOT
 73 #define CCNL_MAX_NONCES                 256 // for detected dups     
 74 #endif //CCNL_RIOT

Why not stick with a ifdef CCNL_RIOT in this case?

[blacksheeep]

I can confirm that this can lead to a memory access violation, highly likely a security issue. We need definitely checks and guards for that.

For the second point, I think the reason is, that the current nonce detection is not really inside the specifications for any packet format. Nevertheless, this is a efficient way to prevent loops. To match the NDN specification, there is nonce deduplication by using the PIT. Thus, by setting the CCNL_MAX_NONCES to -1 we end up matching the Specs, which may be also usable for other usecases and not only for RIOT. (I agree, doc is missing about that). Collecting Nonce instead of only using PIT entries may be more efficient, since PIT entries may timeout.

[NicoleG25]

@blacksheeep was this issue ever addressed? please note that there was a CVE assigned.

[blacksheeep]

No, it's open still.

[NicoleG25]

No, it's open still.

Do you plan on addressing this ? I'm performing some security research and would like to know if that's okay with you :) Thanks and have a nice day !