Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#6157 closed defect (fixed)

CPL_SHA256Update uses recursive function without effects

Reported by: Mateusz Łoskot Owned by: Even Rouault
Priority: normal Milestone:
Component: default Version: unspecified
Severity: normal Keywords:
Cc:

Description

The burnStack function here trunk/gdal/port/cpl_sha256.cpp@31010#L128 has no side effects visible outside its call, so every sane compiler deletes it during optimisation phase. It makes it a dead code.

Unless, the intention is different, then the current implementation may indicate a subtle bug.

Change History (4)

comment:1 by Even Rouault, 8 years ago

This comes directly from http://code.google.com/p/ulib/source/browse/trunk/src/base/sha256sum.c?r=39 . My understanding is that burnStack() attempts to clean the stack variables of CPL_SHA256Guts(). The intend must be to clean sensitive information.

comment:2 by Mateusz Łoskot, 8 years ago

Right, the intention makes sense. The code does not make it into binary anyway, but may cause compilation warnings.

It is an interesting case, by the way. It looks like a brute-force/naive? approach, but modern compilers may need more sophisticated clean up, architecture-specific: http://stackoverflow.com/a/28519486/151641

If you feel the ticket should be closed, I will disable compiler warnings with some pragma tricks,

Last edited 8 years ago by Mateusz Łoskot (previous) (diff)

comment:3 by Even Rouault, 8 years ago

Resolution: fixed
Status: newclosed

Here's an attempt to solve the issue while avoiding assembly tricks...

trunk r31025 "Add size effects to burnStack() so as to hopefully avoid it to be optimized away (#6157)"

comment:4 by Mateusz Łoskot, 8 years ago

Looks good and it fixed the warnings fo rme. Having he code present at run time, also makes it actually more secure.

Thanks Even

Last edited 8 years ago by Mateusz Łoskot (previous) (diff)
Note: See TracTickets for help on using tickets.