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
leaked_storage: Variable "sam" going out of scope leaks the storage it points to.
leaked_storage: Variable "s" going out of scope leaks the storage it points to.
leaked_storage: Variable "snt" going out of scope leaks the storage it points to.
(we overwrite the password and pin arguments).
This implies changes in the argument parsing tests that now must pass a mutable argv
(copied from the statically declared test argvs).
Some other const inconsistency have been dealt with too.
Adds a callback that allows servers to compute NTLM hashes by themselves. The typical
use of this callback is to provide a function that gives precomputed hash values.
Sponsored by: Wheel Systems (http://www.wheelsystems.com)
- fixed invalid, missing or additional arguments
- removed all type casts from arguments
- added missing (void*) typecasts for %p arguments
- use inttypes defines where appropriate
- winpr_HMAC_New() now just returnes the opaque WINPR_HMAC_CTX* pointer
which has to be passed to winpr_HMAC_Init() for (re)initialization
and since winpr_HMAC_Final() no more frees the context you always have to
use the new function winpr_HMAC_Free() once winpr_HMAC_New() has succeded
- winpr_Digest_New() now just returns the opaque WINPR_DIGEST_CTX* pointer
which has to be passed to winpr_Digest_Init() for (re)initialization
and since winpr_Digest_Final() no more frees the context you always have to
use the new function winpr_Digest_Free() once winpr_Digest_New() has succeded
The lpnSize parameter for GetComputerNameEx specifies the total
size of the buffer (in characters).
However, the current code calculated the amount of bytes.
Since only GetComputerNameExA was used and because sizeof(CHAR) == 1
the result was correct but the math was wrong.
Credit goes to @byteboon
On input, the lpnSize [in, out] parameter for GetComputerNameEx()
specifies the total size of the buffer (in characters).
Several functions in ntlm.c were off by one which caused ntlm to fail
if the netbios hostname's strlen was exactly MAX_COMPUTERNAME_LENGTH.
This patch fixes NTLM authentication to work properly on a big endian
machines. Freerdp exited with the following error without recent commits:
[09:50:20:914] [13821:13822] [ERROR][com.freerdp.core.transport] - BIO_read returned an error: error:14094438:SSL routines:ssl3_read_bytes:tlsv1 alert internal error
[09:50:20:914] [13821:13822] [ERROR][com.freerdp.core] - freerdp_set_last_error ERRCONNECT_CONNECT_TRANSPORT_FAILED [0x2000D]
[09:50:20:914] [13821:13822] [ERROR][com.freerdp.client.x11] - Freerdp connect error exit status 1
https://github.com/FreeRDP/FreeRDP/issues/2520
Data in ntlm_av_pair_list are accessed directly, which doesn't work on
big endian machines currently. The recieved data are stored as little
endian. Use conversion macros from endian.h to load and store the data
properly.
https://github.com/FreeRDP/FreeRDP/issues/2520
build-config.h should contain configure/compile time settings that are
relevant for projects that use FreeRDP.
For example the compiled in plugin search paths.
It was supported in freerdp 1.0.2 but not supported in lastest master.
We should take empty password if it is explicitly specified with /v option.
If a password is not specified, we could first try SAM file. If the user entry does not exist, prompt for password.
This patch contains:
* checks for malloc return value + treat callers;
* modified malloc() + ZeroMemory() to calloc();
* misc fixes of micro errors seen during the code audit:
** some invalid checks in gcc.c, also there were some possible
integer overflow. This is interesting because at the end the data are parsed
and freed directly, so it's a vulnerability in some kind of dead code (at least
useless);
** fixed usage of GetComputerNameExA with just one call, when 2 were used
in misc places. According to MSDN GetComputerNameA() is supposed to return
an error when called with NULL;
** there were a bug in the command line parsing of shadow;
** in freerdp_dynamic_channel_collection_add() the size of array was multiplied
by 4 instead of 2 on resize
