Avoid questionable use of the value of a pointer that refers to deallocated space #2897
Conversation
deallocated by a call to the free function in tls_decrypt_ticket.
bernd-edlinger
changed the title
mattcaswell
added
branch: 1.0.2
| @@ -3170,10 +3170,11 @@ static int tls_decrypt_ticket(SSL *s, const unsigned char *etick, | |||
| p = sdec; | |||
|
|
|||
| sess = d2i_SSL_SESSION(NULL, &p, slen); | |||
| slen -= p - sdec; | |||
There was a problem hiding this comment.
need an int cast around the subtraction.
There was a problem hiding this comment.
In some cases, that will still raise warnings. We have a PTRDIFF_T in crypto/evp/evp_enc.c that we probably should make wider use of for these kinds of things.
There was a problem hiding this comment.
No, it has to be an int because slen is an int.
There was a problem hiding this comment.
Apparently there are no gcc and no clang warnings with --strict-warnings about this.
Only visual studio builds have one warning here.
However there are already many similar warnings all over in this function.
Sigh, it feels somehow wrong to defeat a warning here with a type-cast.
A type cast is just not checked at all, and even
slen -= (int)p; would no longer give me any warning.
There was a problem hiding this comment.
Never mind my previous (now removed) comment... d2i_SSL_SESSION() will of course change p...
There was a problem hiding this comment.
The diff between two pointers will invariably cause a warning on VMS when building with 64-bit pointers, because the result is coerced to a 32-bit value, which may lose data. Not a risk in this case, but still an annoying warning. We've taken quite a bit of effort in other parts of our source to avoid this...
There was a problem hiding this comment.
well then, what do folks want to do? @bernd-edlinger is right, it compiles without warning (my mistake). @levitte can VMS live with this PR?
There was a problem hiding this comment.
Worst case scenario, you'll see me adding pragmas in a day or two ;-)
Fine with me.
There was a problem hiding this comment.
Yes, I see, this makes it even worse...
Maybe it would be better to have a function that does the pointer-difference thing
in a clean way, and returns the remaining bytes in the input stream, so I could
use that directly as argument of the next d2i-function.
To resolve this issue, I will add a comment, what this statement is meant to do,
so it will hopefully be more clear, and add a type-cast to int.
The reason why I hate type-casts is: once the interfaces are cleaned up
the type-casts usually remain forever :-(
There was a problem hiding this comment.
It turns out that VMS has no issues here... to my surprise, but I'm not complaining!
|
I'll merge this as-is now. |
|
The commits merged, and Richard just missed the line above that moved p, so don't worry about it. |
deallocated by a call to the free function in tls_decrypt_ticket. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2897)
|
OpenSSL_1_0_2-stable e959b53 Avoid questionable use of the value of a pointer |
This avoids to use the pointers p and sdec after the object is deallocated.
This PR will hopefully be able to use on all branches.