Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mfrey opened this issue Jan 30, 2018 · 5 comments
Open

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

mfrey opened this issue Jan 30, 2018 · 5 comments

Comments

@mfrey
Copy link
Collaborator

mfrey commented Jan 30, 2018

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
Copy link
Collaborator Author

mfrey commented Jan 30, 2018

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
Copy link
Contributor

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.

@blacksheeep blacksheeep changed the title type confusion in ccnl_fwd_handleInterest [CVE-2018-6480] - type confusion in ccnl_fwd_handleInterest Feb 1, 2018
@NicoleG25
Copy link

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

@blacksheeep
Copy link
Contributor

No, it's open still.

@NicoleG25
Copy link

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 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants