[sane-devel] possible bin_w_string security issue (not)
Hi,
On Fri, Oct 15, 2004 at 03:47:06PM +0200, Johannes Berg wrote:
I was going to write a mail without the (not) but then discovered that
this issue is actually a non-issue, but this is totally non-obvious.
Would you know off-hand why?
SIGPIPE (ok, I read your mail before answering) :-)
bin_w_string contains the following code:
if (w-direction == WIRE_DECODE)
{
if (len == 0)
*s = 0;
else if (w-status == 0)
*(*s + len - 1) = '\0';
}
If I interpret the code correctly, this could be used to bring the
server (running the binary protocol) into an invalid state:
Nearly any protocol violation from server or client can have strange
results. That's one of the problems with the SANE net protocol.
Also there are still many tests missing for a bad wire status (see bug
#300105).
* now, the server has a password string that is not zero terminated,
and strcpy()s that string accessing memory beyond the allocated size.
There may be other locations but at least here it tests for the wire
status:
(saned.c l. 356)
sanei_w_authorization_req (wire, req);
if (wire.status)
{
DBG(DBG_ERR, auth_callback: bad status %d\n, wire.status);
return;
}
if (req.username)
strcpy (username, req.username);
[...]
All the wire code expects the caller to test the wire status before he
does anything else. I think there should be tests in all functions of
sanei_net.c also to avoid calling sanei_wire again when the wire is
already bad.
[Actually, I discovered this while I was documenting the wire protocol.
I think it is overly complex and the code badly structured, the wire's
direction thing is really strange!]
I'm still not sure if I have understood all the details of the wire
code. But I guess the idea was to have only one function for each kind
of data. So you don't need one for the server and one for the client
and one for ascii and one for bin and one for sending and one for
receiving...
Ok, so here's the solution:
The server does
signal (SIGPIPE, quit);
(which I think is actually another bug, why should sane_close,
sane_exit, sanei_w_exit and lots of other functions be async signal
safe?)
They aren't. Only sane_cancel() must be async signal safe.
Yes, I guess that's a bug.
Still, this is that non-obvious that someone writing a server (or
client, which could then be attacked by a malevolent server) with the
sanei library functions could easily oversee this detail, ignore SIGPIPE
or use sockets that don't raise it.
Nobody should ever use sanei_wire/sanei_net/sanei_codec_* without
really knowing what he does :-)
Therefore I'm still sending this mail in hope that someone fixes the
sanei_w_array function to check for status != 0 and clean up after
itself.
But what should sanei_w_array do? It already exits when the wire is in
bad state. I think that's really up to the caller.
Bye,
Henning
[sane-devel] possible bin_w_string security issue (not)
--=-yhkGcHckmX4ZO5PEvTHQ Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, SIGPIPE (ok, I read your mail before answering) :-) :-) Nearly any protocol violation from server or client can have strange results. That's one of the problems with the SANE net protocol. =20 Also there are still many tests missing for a bad wire status (see bug #300105). Will do. There may be other locations but at least here it tests for the wire status: Hm. I guess you're right. Seems I missed it, or maybe I was writing this when I looked at a different location? Sorry. I'm still not sure if I have understood all the details of the wire code. But I guess the idea was to have only one function for each kind of data. So you don't need one for the server and one for the client and one for ascii and one for bin and one for sending and one for receiving... Yeah, and then it is also overloaded for freeing allocated data. They aren't. Only sane_cancel() must be async signal safe. =20 Yes, I guess that's a bug. Ok. Nobody should ever use sanei_wire/sanei_net/sanei_codec_* without really knowing what he does :-) :-) But what should sanei_w_array do? It already exits when the wire is in bad state. I think that's really up to the caller. Yes, but before exiting it should free the result buffer and NULL the pointer. When the wire is in an invalid state during reading, it cannot be guaranteed that the buffer is in a valid state, so should be discarded. johannes --=-yhkGcHckmX4ZO5PEvTHQ Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part -BEGIN PGP SIGNATURE- Comment: Johannes Berg (SIP Solutions) iQIVAwUAQXEVXaVg1VMiehFYAQLPOw/9E6+OseXpkvwWNMTenxSGxMF28XX/Rl71 s7sYiR4Fg9hdmPQDy0RtTkQrKspY6izzgn1aGlsH7H2eLw2pIF7yqW36vX2gQXWo kwju+ZF+dIDLWjQ8ICiyuJFgYUNWxjZjDTfLhwe16ByGnVT+nWIx9ZbPYaB1KR0N PmqH3M9G+s7oaZFh5zKtPiJA6kSBbsgpnhSDZpqzop+Rfe8vrFIPN7UTQS2PqZUx 20kMuH4CQu0ggpJ9yekIfzRdbBAZcHSCxKo7vTuupfratbbib9D2firVgW77aaNO Lo3gnue+/TgxLJWeaBxrn/unL/q5U/YnNGhUgAsDBlGymjpZJV+ApOYHoVarv5XS CbyYKKrszzhO8otxfkfA07v7QxqqpDw1CRE2tfxHqXyk2hmyVsy38AHrZOyevMGS D2du8D63miUyVaU3dlGrjmcSxE7pohki9BpMjZ54hIEiF0zw2H9pC1/363KWNzl+ 2rVZcrCmBQMrwoewepWGpvQBJiFZ9sxUkAmaQu/Jv3HEOQnsX8ZmQwKZpKy1qPHm ARqx2Yex/OJ8R8ViKQfPjpRVyBvYfchle+XNRpJh+lbn2VGwh4KKWNGaaoFqdOJd Ybwv5LTz7XtYFmR3IOssFp8BjrTULE5oezwrwCFLqyIJAGC64dGnkdTli6xlEp2E h41UsBd5daQ= =cJpn -END PGP SIGNATURE- --=-yhkGcHckmX4ZO5PEvTHQ--
[sane-devel] possible bin_w_string security issue (not)
--=-8KpxnP81NRNqGGtcVgwN
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable
I was going to write a mail without the (not) but then discovered that
this issue is actually a non-issue, but this is totally non-obvious.
Would you know off-hand why?
bin_w_string contains the following code:
if (w-direction =3D=3D WIRE_DECODE)
{
if (len =3D=3D 0)
*s =3D 0;
else if (w-status =3D=3D 0)
*(*s + len - 1) =3D '\0';
}
If I interpret the code correctly, this could be used to bring the
server (running the binary protocol) into an invalid state:
1) send any kind of request that requires a string option at the end,
for example SANE_NET_AUTHORIZE
2) tell the server that the length is 10 or so
3) send 9 bytes and close the connection
The server will react to this in the following way (if I interpret the
code correctly):
* during sanei_w_array it will notice that the status is
bad and return
* bin_w_string will not(!) zero-terminate the string
(because it is assuming that *s is not valid, while
it actually is)
sanei_w_array doesn't clear the array if anything
item fails to read.
* now, the server has a password string that is not zero terminated,
and strcpy()s that string accessing memory beyond the allocated size.
[Actually, I discovered this while I was documenting the wire protocol.
I think it is overly complex and the code badly structured, the wire's
direction thing is really strange!]
Ok, so here's the solution:
The server does
signal (SIGPIPE, quit);
(which I think is actually another bug, why should sane_close,
sane_exit, sanei_w_exit and lots of other functions be async signal
safe?)
Still, this is that non-obvious that someone writing a server (or
client, which could then be attacked by a malevolent server) with the
sanei library functions could easily oversee this detail, ignore SIGPIPE
or use sockets that don't raise it.
Therefore I'm still sending this mail in hope that someone fixes the
sanei_w_array function to check for status !=3D 0 and clean up after
itself.
johannes
--=-8KpxnP81NRNqGGtcVgwN
Content-Type: application/pgp-signature; name=signature.asc
Content-Description: This is a digitally signed message part
-BEGIN PGP SIGNATURE-
Comment: Johannes Berg (SIP Solutions)
iQIVAwUAQW/U16Vg1VMiehFYAQItGRAApmPf+qNwUfTzFswVVe1K10VSArYVmcOJ
1wN5J3zAhf9oC7gSavY5q2Y5gCZoyCj2+p+iSWf3sjDAV2+RH8WVIS34IItamOC8
zyj9g8/NgHtaF99tTi7/O2j0/3eMvr9t4/dipDZ7l83EI7C0ZYv2CsGX97dmdXf8
HPJo/9Tl4DA+UsBGMngGsj7flVrjf2zThkJzcPqEAgLv8v2Mv4LW5JNtxZIozgGM
LvLdRgYS7SVtt+owY+2bSLdW3V0dYp/QXwqHNXSvZfFHe1FT9AVZBwvF1573J14v
t+T3AwNV/qRxZC4d/0VD6JsPWJA11HFA3agutUK4TN1G10lH+1BkmoM8NXVWWy9p
9jh1G8FsQsPmnV1WGY6jy6JF4rFLARrjfCIefcDKQt5kEC8s9eMiT0+wayvUYZRr
bhO6oOgunMoPpNX3AxyvDhwzZ1uZmoied7QY+C783pY7kIWtZ45mFFCsznnX6FFN
jwhFjubDeyK03SxTzit7KsBy89wTLXqQIaKKR2IqXc9KlXEUp/UGMoEsQSjBBKth
pUSlKymbhp9kSni0DZ8LGX/G9gYhnK0yMxNUYVzBnp8GWuqC/IO/aHX2rR8n0y+W
ztf389uXOpGU7EHKRPW4NfHuzvnW2CZBN9/ReXwkrMPT4K3enB8dAsh3ipj7Ruzh
FaYv+5kkYgU=
=867r
-END PGP SIGNATURE-
--=-8KpxnP81NRNqGGtcVgwN--
