Opened 6 years ago

Closed 6 years ago

Last modified 6 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 Changed 6 years ago by Even Rouault

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 Changed 6 years ago by Mateusz Łoskot

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 6 years ago by Mateusz Łoskot (previous) (diff)

comment:3 Changed 6 years ago by Even Rouault

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 Changed 6 years ago by Mateusz Łoskot

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 6 years ago by Mateusz Łoskot (previous) (diff)
Note: See TracTickets for help on using tickets.