Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#3454 closed enhancement (fixed)

fix truncated HMAC verify

Reported by: fdupont Owned by: fdupont
Priority: low Milestone: Kea0.9
Component: crypto Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Spec is RFC 4635 section 3.1

Subtickets

Change History (16)

comment:1 Changed 6 years ago by tomek

This is a follow up of #2406 review.

comment:2 Changed 6 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.0
  • Priority changed from medium to low

Truncated HMAC is an optional feature in RFC4635. As we don't require this ticket in 0.9, it goes to 1.0 as low (because it is optional).

Obviously, that doesn't mean that someone who is interested in this ticket can't work on it.

comment:3 Changed 5 years ago by fdupont

  • Owner changed from UnAssigned to fdupont
  • Status changed from new to assigned
  • Version set to git

comment:4 Changed 5 years ago by fdupont

Will be stacked over #3471 (because #3471 is about cryptolink code cleanup).

comment:5 Changed 5 years ago by fdupont

Done in 4b9995b..d35d757.

comment:6 Changed 5 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from assigned to reviewing

comment:7 Changed 5 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:8 follow-up: Changed 5 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit d35d7570ca98e4ea0f7c677616276493be342a26

src/lib/cryptolink/botan_hmac.cc
I compared the verify() method against RFC 4635 section 3.1. A few points:

The quoted section talks about the "MAC size" field and "HMAC output length". The method uses "len" and "size", but obtains "size" from the method getOutputLength(). Comments indicating what variable corresponds to what quantity in the quoted section would be helpful.

If the passed signature length is zero, the code explicitly sets it to "size" then passes it to the Botan::same_mem() method. As "len" is the length of valid data pointed to by "sig", does this not risk same_mem() accessing invalid memory?

The section talks about '"MAC size" field is less than the larger of 10 (octets) and half the length of the hash function in use'. There is nothing in the function that seems to apply tthis condition.

I found comparing the logic here to that in RFC 4635 section 3.1 awkward. Following the logic in that section more closely would be helpful, e.g.

if (hmac_length > 0) {
   if (mac_size > hmac_length) {
      return (FALSE);
   } else if (mac_size == hmac_length) {
      do operation;
   } else if (mac_size >= max(10, hmac_length / 2)) {
      truncate mac;
      do operation;
   } else {
      return (FALSE);
   }
} else {
  :
}

comment:9 in reply to: ↑ 8 Changed 5 years ago by fdupont

Replying to stephen:

Reviewed commit d35d7570ca98e4ea0f7c677616276493be342a26

src/lib/cryptolink/botan_hmac.cc
I compared the verify() method against RFC 4635 section 3.1.

=> it is not the right reference: RFC 4635 is about TSIG, cryptolink HMAC reference is RFC 2104.

The quoted section talks about the "MAC size" field and "HMAC output length". The method uses "len" and "size", but obtains "size" from the method getOutputLength(). Comments indicating what variable corresponds to what quantity in the quoted section would be helpful.

=> len is the name of the argument so I use size for the other one. Nothing can make this really clearer, hopefully the code is short enough...

If the passed signature length is zero, the code explicitly sets it to "size" then passes it to the Botan::same_mem() method. As "len" is the length of valid data pointed to by "sig", does this not risk same_mem() accessing invalid memory?

=> len == 0 means to use the whole length, i.e., size. I agree it is not the best API but if you believe it should be fixed (an idea I can share) you should open a ticket asking for explicit and not optional "len" arguments for methods.

The section talks about '"MAC size" field is less than the larger of 10 (octets) and half the length of the hash function in use'. There is nothing in the function that seems to apply tthis condition.

=> verify() returns always false for (len != 0 && len < size / 2). Note it is a sanity measure, these constraints must be applied to the HMAC key declarations.

I found comparing the logic here to that in RFC 4635 section 3.1 awkward.

=> not surprising, RFC 4635 is not the right reference so its logic is different.

comment:10 Changed 5 years ago by fdupont

  • Owner changed from fdupont to stephen

comment:11 Changed 5 years ago by stephen

=> it is not the right reference: RFC 4635 is about TSIG, cryptolink HMAC reference is RFC 2104.

In that case, the description of this ticket is wrong, as that reads "Spec is RFC 4635 section 3.1". I'll re-review it against RFC 2104

comment:12 follow-up: Changed 5 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit 285d83ad66b896728d3471afc473caf79f3339b2 and re-reviewed botan_hmac.cc in light of RFC 2104.

RFC 2104 section 5 states:

We recommend that
the output length t be not less than half the length of the hash
output (to match the birthday attack bound) and not less than 80 bits
(a suitable lower bound on the number of bits that need to be
predicted by an attacker).

The HMACImpl::verify() function contains the code:

            Botan::SecureVector<Botan::byte> our_mac = hmac_->final();
            size_t size = getOutputLength();
            if (len != 0 && len < size / 2) {
                return (false);
            }

Shouldn't that check be:

if (len != 0 && len < std::max((size / 2), 10)) {

... to take into account the check for 80 bits?

comment:13 in reply to: ↑ 12 Changed 5 years ago by fdupont

Replying to stephen:

Shouldn't that check be:

if (len != 0 && len < std::max((size / 2), 10)) {

... to take into account the check for 80 bits?

=> perhaps but in this case:

  • the OpenSSL code should be changed too
  • the std::max should be expanded (both to make the code more readable and to avoid conflict with a max() macro, cf WIN32...).

I'll do both during the merge (as the 2 other tickets are ready)?

comment:14 Changed 5 years ago by fdupont

Merged to #920 ticket.

comment:15 Changed 5 years ago by fdupont

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:16 Changed 5 years ago by hschempf

  • Milestone changed from Kea1.0 to Kea0.9
Note: See TracTickets for help on using tickets.