Skip to content

Add support for HMAC based on sha256 (128b keys). #669

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kjopek
Copy link

@kjopek kjopek commented Mar 27, 2018

Add support for HMAC based on SHA256 with 128 bits keys.

It can be used in the following form:
./tcpdump -nr ~/ipsec_dump.pcap -s0 -E "[email protected] aes256-cbc-hmac-sha256-128:[KEY1],[email protected] aes256-cbc-hmac-sha256-128:[KEY2]"

Sponsored by: Digital Fingerprints

Also extract duplicated code into local function.

Sponsored by: Digital Fingerprints.
Copy link
Member

@mcr mcr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good changes, but it needs some test cases to exercise sha256.
Even if you haven't got any ESP packets, at least, you can instantiate the hash and it can fail to authenticate the packets in question.

@fxlb
Copy link
Member

fxlb commented Jul 10, 2021

And please, rebase on top of the-tcpdump-group:master.

@infrastation
Copy link
Member

In addition to the previous comment, which still stands (merging from 3 years ago is not rebasing), you might want to read the file CONTRIBUTING.md (especially step 4), so you have a compiler feedback loop in your working copy.

@kjopek
Copy link
Author

kjopek commented Jul 11, 2021

Thank you very much for all your comments and I am very sorry for breaking contributors' rules.

From what I see now, after 3+ years, this patch was wrong from the very beginning and here is my explanation why I think so.

The original patch was kind-of quick and therefore dirty hack to allow IPSEC debugging under certain configuration. Particularly, to make sure the encryption works as expected and data is correctly encrypted/decrypted. In my humble opinion, the pull request should have been splitted into at least two smaller ones:

  1. First one to introduce strendswith function and use it where needed.
  2. Second one to introduce additional primitive for packet authentication.

While the first one is rather obvious, the second one is tricky as the whole function relies on authlen variable which is used only for keeping length of hmac code and is required only to calculate correct offsets in the packet to decrypt it correctly. I see no other usage of authentication-related code in print-esp.c file. sa_list structure maintains authsecret and authsecret_len fields which are unpopulated and therefore does not play any role in printing ESP packets. In my humble opinion, tcpdump lacks hmac verification and this functionality could be added separately.

Summing up, my original intention was to enable ESP decryption for different ENC+AUTH scenario, but from my current perspective this pull request should be closed and I should create another one only to implement strendswith function. The remaining part (lack of auth support) should be discussed and added separately. I am leaving this PR open to allow further discussion.

@infrastation
Copy link
Member

Thank you for the detailed comment. If you think it would make more sense to split the change into two commits, split it. If the first commit is useful on its own, it can be merged on its own. Would you like to take more time to state the decryption problem properly?

@ZsBT
Copy link

ZsBT commented Mar 20, 2023

+1 for this (and keeping the issue open)

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

Successfully merging this pull request may close these issues.

5 participants