[sane-devel] possible bin_w_string security issue (not)

[Henning Meier-Geinitz]

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