There were two main issues here: First, the `ntlm_av_pair_add` and
`ntlm_av_pair_add_copy` were not adding a new `MsvAvEOL` to the end of
the list to replace the one they overwrote. This caused the second call
to one of those functions to fail (since it couldn't find the
terminator), which was the source of the test failure. It also caused
`ntlm_av_pair_list_length` and `ntlm_print_av_pair_list` to read out of
bounds until they happened to find the right word.
Second, several bounds checks were wrong or missing. For example,
`ntlm_av_pair_add` does not ensure that the value fits inside the list.
And `ntlm_av_pair_get_len` and `ntlm_av_pair_get_value_pointer` can
return error codes or NULL, but those error returns were ignored, and
the values used anyway (such as in `ntlm_av_pair_add_copy`).
This fixes the list handling code to have the invariant that all
functions returning `NTLM_AV_PAIR*` only return non-`NULL` if the entire
returned `AvPair` is within bounds. This removes the need for the length
parameter in functions that only operate on a single `AvPair`. This
check is performed by the new `ntlm_av_pair_check` helper, which is
added in some new places and used to simplify the code in others.
Other issues fixed along the way include:
- `ntlm_av_pair_list_length` did not cast to `PBYTE`, so it was
returning the number of `NTLM_AV_PAIR`-sized chunks (which was
possibly not even an integer) instead of the number of bytes
- I removed an impossible check for `offset <= 0` in
`ntlm_av_pair_get_next_pointer`
- The assertion that `Value != NULL` and the call to `CopyMemory` are
only necessary if `AvLen` is nonzero
- `ntlm_av_pair_get_next_pointer` (renamed to `ntlm_av_pair_next`)
could be declared `static`
With this commit, TestNTLM now passes on powerpc64.
```
$ ./Testing/TestSspi TestNTLM
NTLM_NEGOTIATE (length = 40):
NTLM_CHALLENGE (length = 168):
NTLM_AUTHENTICATE (length = 352):
$ echo $?
0
```
Fixes#5250
found by cppcheck
[client/Windows/wf_event.c:464] -> [client/Windows/wf_event.c:468]: (warning) Variable 'xNewPos' is reassigned a value before the old one has been used. 'break;' missing?
* When senging a close request for a dynamic channel close the channel
immediately. A response from the server is not guaranteed.
* Use the *_PDU defines instead of magic numbers when creating
dynamic channel messages.
The assistance file requires primitives from the ssl wrapper.
Enable these before parsing the file.
Additionally split the FIPS mode enablement from the one time
initializer to avoid ignoring that flag.
found by cppcheck
[libfreerdp/codec/include/bitmap.c:112] -> [libfreerdp/codec/include/bitmap.c:95]: (warning) Either the condition '!pbSrcBuffer' is redundant or there is pointer arithmetic with NULL pointer.
[libfreerdp/codec/include/bitmap.c:112] -> [libfreerdp/codec/include/bitmap.c:96]: (warning) Either the condition '!pbDestBuffer' is redundant or there is pointer arithmetic with NULL pointer.
The value INVALID_HANDLE_VALUE could in theory be a valid memory address,
so the analyzer is confused and thinks either we have a memroy leak
or we try to free a fixed address.
