Opened 9 months ago

Closed 9 months ago

#28120 closed defect (fixed)

hs_descriptor: CID 1440368: Incorrect expression (SIZEOF_MISMATCH)

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: coverity tor-hs
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:


*** CID 1440368:  Incorrect expression  (SIZEOF_MISMATCH)
/src/feature/hs/hs_descriptor.c: 2954 in hs_desc_build_authorized_client()
2948                                                   HS_DESC_COOKIE_KEY_BIT_SIZE);
2949       /* This can't fail. */
2950       crypto_cipher_encrypt(cipher, (char *) client_out->encrypted_cookie,
2951                             (const char *) descriptor_cookie,
2952                             HS_DESC_DESCRIPTOR_COOKIE_LEN);
>>>     CID 1440368:  Incorrect expression  (SIZEOF_MISMATCH)
>>>     Passing argument "keystream" of type "uint8_t *" and argument "8UL /* sizeof (keystream) */" to function "memwipe" is suspicious.
2954       memwipe(keystream, 0, sizeof(keystream));
2955       tor_free(keystream);

Child Tickets

Change History (6)

comment:1 Changed 9 months ago by asn

Caused by #27549 changes.

The issue is:

  uint8_t *keystream = NULL;
  memwipe(keystream, 0, sizeof(keystream));

that is, we use the sizeof the pointer to memwipe, instead of the actual length of the keystream array.

The patch would have been trivial but the issue is that the length of the keystream array
is hidden inside build_descriptor_cookie_keys(). Should we break the layering and also compute the length out of that function? Or should we make a new function that wipes the cookie key and is aware of the length? Or what?

comment:2 Changed 9 months ago by dgoulet

Probably our here is to pass the keystream length to build_descriptor_cookie_keys(). At first glance, it does seems like that function has no business of computing by itself the length but rather it would be the caller's job.

comment:3 Changed 9 months ago by nickm

Milestone: Tor: 0.3.5.x-final

comment:4 Changed 9 months ago by asn

Status: newneeds_review

see PR:

bug not in any released tor version so no changes file included.

comment:5 Changed 9 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready


comment:6 Changed 9 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

ok, merged!

Note: See TracTickets for help on using tickets.