Re-implement below change but for the legacy ccmp decryption used on
3.2 release track but also on later releases when Wireshark is built
with older versions of libgcrypt:
e5e37add9a 802.11 Decrypt: Fix AAD Calculation when HT-Control present in a QoS Data Frame
Ping #17577.
The current implementation misses an edge case, where bit 15 of the FC Field
must be zeroed in case a QoS Control field is present (IEEE 802.11 12.5.3.3.3),
which results in a wrong AAD and subsequently in the failure of the packet
decryption for QoS Data Frame carrying a HT Control Field (indicated by bit
15 set to 1).
In addition the field 14 must be 1, which is ensured explicitly (the author is
not sure if the code is reachable by frames which do not have bit 14 set to
one).
The debug log levels used in dot11decrypt are pretty much random.
Cleanup how the different levels are used and at the same time change
to standard wslog log levels.
With this change log levels are used as follows:
WARNING - Allocation failures or unexpected (but handled) errors.
DEBUG - Debug messages mainly related to key derivation.
NOISY - Debug messages related to packet decryption.
The GTK key provided in FT BSS Transition IE is encrypted. Update
dot11decrypt engine to return the decrypted key for dissection.
Change-Id: Id31a8cf77e12568f2e449470822a64792895673c
Support decrypting captures with Fast BSS Transition roaming present
by now also scanning (re)association frames for relevant information
elements and feeding it into the dot11decrypt engine.
Both (re)association request and response frames are scanned to allow
for potentially missing one frame and still be able to derive PTKs
needed for successful decryption.
Closes#17145
Change-Id: I08436582e4f83695dc606ddb92ff442d6258ef9b
When building with GCC 10.2.0 and optimization level 3 some new
warnings turn up. Fix them.
./epan/crypt/dot11decrypt_util.c: In function ‘dot11decrypt_derive_pmk_r0’:
../epan/crypt/dot11decrypt_util.c:308:5: error: ‘sha256_res’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
308 | memcpy(pmk_r0_name, sha256_res, 16);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../epan/crypt/dot11decrypt_util.c: In function ‘dot11decrypt_derive_pmk_r1’:
../epan/crypt/dot11decrypt_util.c:357:5: error: ‘sha256_res’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
357 | memcpy(pmk_r1_name, sha256_res, 16);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../wiretap/wtap_opttypes.c: In function ‘wtap_block_add_if_filter_option’:
../wiretap/wtap_opttypes.c:782:12: error: ‘*((void *)&filter_dest+8)’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
782 | return filter_dest;
| ^~~~~~~~~~~
../wiretap/wtap_opttypes.c: In function ‘wtap_block_set_if_filter_option_value’:
../wiretap/wtap_opttypes.c:782:12: error: ‘*((void *)&filter_dest+8)’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
782 | return filter_dest;
| ^~~~~~~~~~~
I believe this was the original intention, to use these API restricitons
with dissectors only (not that I necessarily agree with that policy either),
and through copy-paste and lack of clear guidelines it spread to other
parts of the build.
Rename the checkAPI groups to make it very clear that this is dissector-only.
This doesn't mean, of course, that good programming practices shouldn't be
followed everywhere. In particular assertions need to be used properly.
Don't use them to catch runtime errors or validate input data.
This commit will be followed by another removing the various ugly hacks
people have been using to get around the checkAPI hammer.
The include_directories documentation at
https://cmake.org/cmake/help/latest/command/include_directories.html
says:
"Note: Prefer the target_include_directories() command to add include
directories to individual targets and optionally propagate/export them
to dependents."
Switch from include_directories to target_include_directories in a bunch
of places.
Add "SYSTEM" to the remaining external include_directories calls in
order to minimize our compiler warning blast radius.
Use target_include_directories instead of include_directories in a few
places as recommended at
https://cmake.org/cmake/help/latest/command/include_directories.html
Doing so lets us mark a bunch of dependency includes SYSTEM PRIVATE, in
particular LIBXML2_INCLUDE_DIRS. On macOS this keeps us from triggering
the nullability warnings described at
https://www.wireshark.org/lists/wireshark-dev/202004/msg00056.html
(This might also keep the Visual Studio code analyzer from complaining
about various Qt headers, but I haven't tested this.)
dot11decrypt.c:1775:27: error: ‘ptk_len’ may be used uninitialized in
this function [-Werror=maybe-uninitialized]
sa->wpa.ptk_len = (INT)ptk_len;
^
Change-Id: I705007a8b351c333dc1c8cb1d455ea2f0976c964
Try to fix
run/libwireshark.so.0.0.0: undefined reference to `dot11decrypt_derive_pmk_r1'
run/libwireshark.so.0.0.0: undefined reference to `dot11decrypt_kdf'
run/libwireshark.so.0.0.0: undefined reference to `dot11decrypt_prf'
run/libwireshark.so.0.0.0: undefined reference to `dot11decrypt_derive_pmk_r0'
run/libwireshark.so.0.0.0: undefined reference to `dot11decrypt_derive_ft_ptk'
The Dot11DecryptRsna4WHandshake has grown unreasonably large. Refactor
the function and break out some functionality into smaller utility
functions to make it easier to understand.
Add partial support for decrypting captures with connections
established using FT-EAP. To support deriving keys for FT-EAP
the MSK is needed. This change adds MSK as a valid IEEE 802.11
protocol input key type preference as well.
Note that FT-EAP support comes with the following imitations:
- Keys can only be derived from the FT 4-way handshake messages.
- Roaming is not supported.
Add partial support for decrypting captures with connections
established using FT BSS Transition (IEEE 802.11r).
FT BSS Transition decryption comes with the following limitations:
- Only FT-PSK is supported.
- Keys can only be derived from the FT 4-way handshake messages.
- Roaming is not supported.
Break out the PRF and KDF functionality from the PMK to PTK
derivation functions and make them separate utility functions
implemented as defined in the IEEE 802.11 standard.
This change is done in preparation for supporting additional
AKMS where the key derivation functions can be reused to derive
other type of keys.
With RHEL/CentOS 6 EOL and already unsupported by Wireshark, there's no
reason to keep the minimum version of libgcrypt below 1.5.0 (which was
released 9.5 years ago). Version 1.6.0 is a big improvement in functionality,
but RHEL/CentOS 7 is stuck on 1.5.3 (As an aside, GCRYPT_VERSION_NUMBER
wasn't defined until 1.5.1, so this change will make us actually use the
libgcrypt AES-WRAP handling on 1.5.0)
Adds a pre-commit hook for detecting and replacing
occurrences of `g_malloc()` and `wmem_alloc()` with
`g_new()` and `wmem_new()`, to improve the
readability of Wireshark's code, and
occurrences of
`g_malloc(sizeof(struct myobj) * foo)`
with
`g_new(struct myobj, foo)`
to prevent integer overflows
Also fixes all existing occurrences across
the codebase.
Instead of using a single memcpy which relies
upon struct members being in contiguous
memory, use three separate calls to memcpy.
This fixes Coverity 1460754.
For WPA security association (SA) entries are created on sucessful
PTK derivation from 4-way handshake frames. WEP though don't use
4-way handshake frames for key derivation and therefore no SA entry
is created. Still WEP decryption implementaton expects to find
an SA otherwise the decryption is skipped.
Fix broken WEP decryption by removing the check for an existing SA
entry and instead form the SA on first successful decryption.
Add also a test for WEP decryption.
Fixes: v3.3.0rc0-1263-g099d241046 ("dot11decrypt: Avoid allocating SA on packet decryption")
Remove the --check-addtext and --build flags. They were used for
checkAddTextCalls, which was removed in e2735ecfdd.
Add the sources in ui/qt except for qcustomplot.{cpp,h}. Fix issues in
main.cpp, rtp_audio_stream.cpp, and wireshark_zip_helper.cpp.
Rename "index"es in packet-usb-hid.c.
Add support for TK user input keys. With this Wireshark can
decrypt packet captures where 4WHS frames are missing and
packet captures with non-supported AKMS, for example
802.11r / Fast BSS Transitioning.
Decryption using user TK works as a backup if the normal
decryption flow does not succeed. Having TK decryption keys
added will affect general IEEE 802.11 dissector performance
as each encrypted packet will be tested with every TK.
Worst case scenario is plenty of TKs where none of them
matches encrypted frames.
On successful user TK decryption an SA is formed based on
parameters used to decrypt the frame. This SA is similar to
what is formed when Wireshark detects and derive keys from
4WHS messages. With the SA entry in place the decryption
performance (success case) should be on par with "normal"
decryption flow.
Bug: 16579
Change-Id: I72c2c1e2c6693131d3ba07f8ddb8ff772c1b54a9
Reviewed-on: https://code.wireshark.org/review/37217
Petri-Dish: Anders Broman <a.broman58@gmail.com>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
The function used to search for an SA allocates one entry if none
exists. Searching for an SA is done in many places including on
packet decryption. In practise this means for every encrypted packet
with unique STA/BSSID an SA is allocated. This is a waste both from a
memory and performance point of view but also a limitation as with
the old static array SA storage decryption would fail when max
number of SAs is reached. i.e. decryption would fail for captures
with more than 256 unique STA/BSSids.
Separate the searching for SA entries and allocation of new SAs to
avoid allocating unnecessary SA entries.
Change-Id: I7ddc9ac4bad5d69e273f97f8f8fb38d34b59a854
Reviewed-on: https://code.wireshark.org/review/37308
Petri-Dish: Anders Broman <a.broman58@gmail.com>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
Simplify the SA storage by replacing the static array with a
hash table. This way there's no need to keep track of whether
an entry is used or not and no need to traverse the whole
array for the non-matching case. This change should benefit
performance but was mainly done to prepare for coming changes
where code adding and searching for SA entries is modified. With
this change in place those changes become cleaner.
Change-Id: Ide572c5e4e7e872f1654d8d8f288cd6451f04435
Reviewed-on: https://code.wireshark.org/review/37307
Petri-Dish: Anders Broman <a.broman58@gmail.com>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
The function tracing functions DEBUG_TRACE_START/END are only
used in a few functions and START/END often don't match making
them unreliable. Remove these macros and their usage as it
clutters code without adding any useful debug capabilities.
Change-Id: I7ea214c07ba1f35cc546942b5d4737f5752d20a7
Reviewed-on: https://code.wireshark.org/review/37306
Petri-Dish: Anders Broman <a.broman58@gmail.com>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
Remove some unused defines, function parameters and functions.
Change-Id: I1bbc3ff7e0a9d11e8521ddf24b35113d8e332f08
Reviewed-on: https://code.wireshark.org/review/37305
Petri-Dish: Anders Broman <a.broman58@gmail.com>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
Coverity Scan reports an out-of-bounds acccess on memcopy from addr1
of 802.11 mac header. This out-of-bounds access is a controlled access
knowing that addr2 and addr3 is located in memory right after addr1.
Type cast to a guint8 pointer to indicate that there's no fix length.
This should silent the Coverity Scan error reported.
Coverity CID 1460754
Change-Id: Ief2280f1b686deebf3aba74f19a5730c66d4d313
Reviewed-on: https://code.wireshark.org/review/36706
Petri-Dish: Anders Broman <a.broman58@gmail.com>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
dot11decrypt_util.c:34:6: warning: no previous prototype for ‘dot11decrypt_construct_aad’ [-Wmissing-prototypes]
Change-Id: Ideda2d18de88aed9d3fd045a02ead6446b0dbfce
Reviewed-on: https://code.wireshark.org/review/36653
Petri-Dish: Anders Broman <a.broman58@gmail.com>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
dot11decrypt_system.h:274:11: warning: parameter 'decrypt_data' not found in the function declaration [-Wdocumentation]
dot11decrypt_system.h:277:11: warning: parameter 'decrypt_len' not found in the function declaration [-Wdocumentation]
dot11decrypt_system.h:279:11: warning: parameter 'key' not found in the function declaration [-Wdocumentation]
Change-Id: I9a0d1dd29ddd130b9425a38645777e4eb05e9fa3
Reviewed-on: https://code.wireshark.org/review/36540
Reviewed-by: Mikael Kanstrup <mikael.kanstrup@sony.com>
Reviewed-by: Anders Broman <a.broman58@gmail.com>
MFP enabled connections with SHA-256 key management (IEEE 802.11w) use
EAPOL key version == 3. This case was missing making decryption of such
connections fail. Allow key version 3 to handle these too.
Change-Id: If9e3fcc5c3bbfb46e82b39dfed5b2a74787a4f16
Reviewed-on: https://code.wireshark.org/review/36534
Reviewed-by: Peter Wu <peter@lekensteyn.nl>
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
From version 1.5.0 of libgcrypt there's support for AES unwrap. Use the
libcrypt function when available.
While at it also make AES_unwrap a static function of dot11decrypt.c
Change-Id: I4f69a766df3ea19ce25122e8d2fd1086f440995b
Reviewed-on: https://code.wireshark.org/review/36431
Petri-Dish: Anders Broman <a.broman58@gmail.com>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
The function to construct AAD is same for both CCMP and GCMP so move
it to a new internal utility c file for later use by both
implementations.
Change-Id: I8c8ffe0f492d5860e2bcd266b4d936383598b47e
Reviewed-on: https://code.wireshark.org/review/36358
Petri-Dish: Anders Broman <a.broman58@gmail.com>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
Simplify the CCMP decryption implementation by letting libgcrypt
handle decryption, authentication and integrity check. This aims
to simplify the implementation in preparation for CCMP-256
decryption support where changes to the CCMP decryption implementation
is anyway needed.
Even though performance optimization was not the target for this
change it appears decryption speed is improved as well.
Change-Id: I6c36315291672f6313c9303ab8e50afb87aea9ce
Reviewed-on: https://code.wireshark.org/review/36343
Petri-Dish: Alexis La Goutte <alexis.lagoutte@gmail.com>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
When decrypting a frame the decryption occurs in a temporary buffer.
After successful decryption the decrypted frame is first copied back,
then a memmove operation is used to remove the CCMP header mid frame.
As the mac header is not encrypted there's no need to copy that part
back again after decryption. This means there's no mid frame data
that must be removed. Instead just copy the relevant portion and
save one memmove operation.
Change-Id: I24b938a6f5fac5a23cd0132aefe9ce258b352ef8
Reviewed-on: https://code.wireshark.org/review/36342
Reviewed-by: Anders Broman <a.broman58@gmail.com>
The offset parameter to functions Dot11DecryptWepMng and Dot11DecryptRsnaMng
is always same as mac_header_len so not needed anymore.
Change-Id: I298e207c9317051b634aabd3f6a0e0921687b4d4
Reviewed-on: https://code.wireshark.org/review/36341
Reviewed-by: Anders Broman <a.broman58@gmail.com>
The temporary buffer to store calculated mic is too short to keep
the message digest when using HMAC-SHA384 algo. HMAC-SHA384 yields
a message digest of 48 bytes so increase buffer size to make room
for the largest possible value.
Ping-Bug: 16197
Change-Id: I36fd094c39ce77329fb303fa181d286be694ae65
Reviewed-on: https://code.wireshark.org/review/35067
Reviewed-by: Anders Broman <a.broman58@gmail.com>
Martin Mathieson