Rename esp_print_decodesecret() to esp_decodesecret_print() and
esp_print_decrypt_buffer_by_ikev2() to esp_decrypt_buffer_by_ikev2_print()
with _print suffix like in most similar cases.
At least as I read RFC 5996 section 3.14 and RFC 4303 section 2.4, if
the cipher has a block size of which the ciphertext's size must be a
multiple, the payload must be padded to make that happen, so the
ciphertext length must be a multiple of the block size. Instead of
allocating a buffer, copying the ciphertext to it, and padding it to the
block size, fail if its size isn't a multiple of the block size.
(Note also that the old padding code added a block's worth of padding to
the end of a ciphertext block that *was* a multiple of the cipher block
size; this might have caused problems.)
Don't use the undocumented EVP_Cipher(); the lack of documentation means
a lack of information about whatever requirements it might impose. Use
EVP_DecryptUpdate() instead.
Before calling it, use EVP_CIPHER_CTX_set_padding() to say "don't do
your own padding, this block is a multiple of the cipher block size".
Instead of using EVP_CipherInit() or EVP_CipherInit_ex(), use
EVP_DecryptInit() or EVP_DecryptInit_ex(). as we're always doing
decryption and never doing encryption - the extra parameter to
EVP_CipherInit() and EVP_CipherInit_ex() is always 0.
This may address GitHub issue #814.
It may also make it a bit easier to have the code use Common Crypto on
macOS (rather than requiring that OpenSSL be installed - macOS ships
with an OpenSSL shared library for binary compatibility with older
releases, but doesn't ship with the headers, because Apple wants you
using their crypto code) and use Cryptography API: Next Generation on
Windows (Vista/Server 2008 and later) (rather than requiring a Windows
build of OpenSSL).
(Hopefully this will all work with LibreSSL.)
If you split a string by overwriting the separator character with a NUL,
and you want to compare the first part with various values, use a
pointer to the beginning of the string, not to the NUL you just dropped
in.
This is a bug found while testing some cleanups for GitHub issue #814;
it's necessary for the cleanups to work.
Some of them are locale-dependent, and all of them run the risk of
failing if you hand them a char with the 8th bit set.
Move our replacements to a new netdissect-ctype.h file, and, for the
ones that check for particular character types, add _ASCII to the name,
to indicate that only ASCII characters pass the check. Do the same for
the ones that map between cases, to indicate that they only map ASCII
letters.
For isspace(), explicitly check for the characters we care about, to
make it clearer what we're doing.
If we have an Ethernet packet where the last 2 octets of the header are
a length rather than an Ethernet type, and it's less than the remaining
length of the packet, shorten the length and captured length, update the
snapshot end.
Turn the buffer stack into a "packet information" stack, so that, if we
*do* update the snapshot end, we push the old end onto the stack, and
pop it off as soon as we're done dissecting the Ethernet packet, in case
there's more data in the packet after the Ethernet packet.
Use the stack when we use the IPv4 and IPv6 length fields as well.
If a dissector has to process its input - decryption, decompression,
etc. - rather than dissect the raw input, it should push the processed
input onto the buffer stack. As soon as the dissection is done, the
stack should be popped, to free the buffer into which the processing was
done, and restore the "pointer to packet data" and "pointer to end of
packet data" members of the netdissect_options structure, so the code
can go back to dissecting the original data.
The stack will get everything popped off it when dissection is done.
Use this mechanism in the ESP decryption code rather than scribbling on
top of the input packet data.
"ivoff" is a pointer to the IV, not the offset of the IV; call it ivptr.
Have a variable that points to the beginning of the ciphertext, and use
that.
Fix the check that makes sure the authentication data/integrity check
value length isn't too big - it needs to make sure that it doesn't go
before the beginning of the ciphertext, i.e. doesn't overlap with the
IV.
Don't bother with a variable pointing to the secret, just pass
sa->secret.
Fix the check that makes sure the padding length isn't too big - make
sure it, plus 2 for the padding length and next header bytes, isn't
bigger than the ciphertext length.
Update a test to reflect the stricter length checks.
Just check whether the pointer to it is within the available packet data
- and, if it's not, report truncation.
While we're at it:
Make the initialization vector length unsigned; it's either zero or a
positive number.
Rename a variable used for the ciphertext len to ctlen.
If we can't decrypt the payload, we can't dissect it, so don't try -
just give up immediately.
While we're at it:
If EVP_CIPHER_CTX_new() fails, it means a memory allocation failed;
treat that as such.
Use some of the arguments we're passed rather than re-fetching them from
the IP header.
Add some comments.
Call nd_print_trunc() for failed length sanity checks, and note that
they can fail due to the decryption being done with the wrong key.
Update one test's output; it is, I think, being decrypted with the wrong
key.
The exceptions are currently:
Some EXTRACT_ in print-juniper.c, not used on packet buffer pointer.
An EXTRACT_BE_U_3 in addrtoname.c, not always used on packet buffer
pointer.
Found with -Wunreachable-code-return clang compiler option.
The errors were:
./print-esp.c:317:10: warning: 'return' will never be executed
[-Wunreachable-code-return]
return 0;
^
./print-esp.c:552:4: warning: 'return' will never be executed
[-Wunreachable-code-return]
return;
^~~~~~
Found with -Wunreachable-code clang compiler option.
The errors were:
./print-esp.c:263:3: warning: code will never be executed
[-Wunreachable-code]
free(input_buffer);
^~~~
./print-esp.c:246:3: warning: code will never be executed
[-Wunreachable-code]
EVP_CIPHER_CTX_free(ctx);
^~~~~~~~~~~~~~~~~~~
./print-esp.c:843:5: warning: code will never be executed
[-Wunreachable-code]
free(input_buffer);
^~~~
./print-esp.c:826:5: warning: code will never be executed
[-Wunreachable-code]
EVP_CIPHER_CTX_free(ctx);
^~~~~~~~~~~~~~~~~~~
This can prevent bizarre failures if, for example, you've done a
configuration in the top-level source directory, leaving behind one
config.h file, and then do an out-of-tree build in another directory,
with different configuration options. This way, we always pick up the
same config.h, in the build directory.
Add some RFC numbers.
Structures with nd_ types work well if you overlay them on top of the
packet buffer, but not as well with an on-the-stack structure into which
you copy from the packet, which is ugly if you do *both* with the same
structure. Use overlaying uniformly.
Add EXTRACT_ macros as necessary for the nd_ types.
Make stuff unsigned if it's appropriate.
Use unsigned formats for unsigned values.
A number of routines are passed the length of a payload, so they don't
have to re-fetch that length themselves. That length has been
pre-checked to make sure it's big enough for the payload header; note
that in comments.
Now all the macros have a name meaning a count in bytes.
With _S_: signed, _U_: unsigned
e.g.:
EXTRACT_BE_32BITS -> EXTRACT_BE_U_4
EXTRACT_LE_32BITS -> EXTRACT_LE_U_4
...
EXTRACT_BE_INT32 -> EXTRACT_BE_S_4
and have:
EXTRACT_8BITS -> EXTRACT_U_1
EXTRACT_INT8 -> EXTRACT_S_1
Get rid of casts to (int) that aren't needed or wanted.
If a field is unsigned, use an unsigned variable for it, print it with
%u, not %d, and don't cast it to int.
Replace a static variable in print-dvmrp.c with a local variable in
dvmrp_print() and a parameter to print_neighbors2().
Attempt to allocate the buffer before doing any decryption calls and, if
the attempt fails, report an error and return an error indication.
Make sizes unsigned, as they can't be negative;
EVP_CIPHER_CTX_block_size() returns an int, but that's just because they
picked the wrong data type in OpenSSL.
No need to use calloc() for the output buffer - just use malloc().
We don't want to try to suppress warnings about discarding the const
qualifier, as that's something we need to fix. Adding the extra
variable doesn't suppress it on some platforms, so it's not useful
there, and if it does suppress it, we don't want that.
And we weren't doing it in the other case, anyway.
Comment in both cases with an XXX comment about the cast.
