math: p_median: Fix for stack overflow weakness.

[ghost]

The use of variable length arrays (VLA) without any size range checking is a security weakness that can be used to provoke a stack overflow that may lead to arbitrary code execution.

My proposed fix simply uses dynamic memory allocation over VLA.

For more information regarding proper use of VLA, please read:
https://www.securecoding.cert.org/confluence/display/c/ARR32-C.+Ensure+size+arguments+for+variable+length+arrays+are+in+a+valid+range

Signed-off-by: Giancarlo Canales Barreto [email protected]

[ghost]

This problem would also limit the number of elements in the vector to about RLIMIT_STACK/sizeof(float), anything bigger than that would cause a stack overflow.

For instance, RLIMIT_STACK on x64 is around 8MB. This means that in the best-case scenario, a vector that has over 2 million elements will cause a stack overflow.

This problem isn't much about trusting the programmer, and more about trusting the source of the data that you want to compute the median of.

[ghost]

@ebadi is right in that int n is incorrect. It should be size_t n.

[lfochamon]

According to QUESTIONS, these situations should be delt as GIGO. PAL should not protect anyone against themselves. However, VLAs are C99 and are not supported by all compiler (Microsoft, notably). I think using malloc instead of VLAs would be better (or better yet, p_malloc when it is implemented).

[mateunho]

@lchamon 👍

[aolofsson]

Security is not a first order concern at this low level library. Assumption is that it will be handled at a higher level. Also, as @ichamon stated there are bound to be portability issues across tools/archs if this is baked into this library.

[lfochamon]

@aolofsson: The current implementation uses VLAs. This PR corrects that (I think I expressed myself very poorly in my comment!).

Also, it might be a good idea to fix the p_malloc and p_free stubs and make them use malloc and free from stdlib for now. This way they can start being used in the rest of the library.