Bug#854804: [sane-devel] Bug#854804: saned: SANE_NET_CONTROL_OPTION response packet may contain memory contents of the server

[Olaf Meeuwissen]

Hi Zdenek,

I really appreciate your efforts to come up with a better patch that
what I have posted to the list. To be honest, I don't really like my
patch but it's the best I could come up with without a testsuite (or
setting up a test environment myself for which I don't have time now
anyway).

Read on, there's more at the bottom :-)

Zdenek Dohnal writes:

> On 03/05/2017 10:40 AM, Olaf Meeuwissen wrote:
>> Hi Zdenek,
>>
>> Zdenek Dohnal writes:
>>
>>> I tried to enhanced Olaf's patch and I posted it here:
>>>
>>> https://paste.fedoraproject.org/paste/qssgq4s0Vtqw6R5wkDWoEV5M1UNdIGYhyRLivL9gydE=
>>>
>>> Were my thoughts right and will it solve this issue?
>> Thinking you just "backported" the patch (it applies with a fuzz but
>> otherwise cleanly against 1.0.25) and removed the comments, I almost
>> overlook your code change!
>>
>> I think it's my FIXME that misled you but you should *not* substract
>> req.value_size.  [...]
>>
>> Hope this clarifies a bit,
>
> Thank you so much for explanation, Olaf. I did not notice that fact
> about req.value_size. So what about fetching string length from
> sanei_w_array function by parameters sent by reference? Is it acceptable
> to change number and type of parameters of functions? I created patch
> proposal:
>
> https://paste.fedoraproject.org/paste/KVJpdlIAMcxiovnYF4dhbV5M1UNdIGYhyRLivL9gydE=
>
> It is probably not final version, but I hope I demonstrated my idea. It
> was compiled without error.

Whether it compiles or not is not the important part ;-)
It's gotta work!  Are you able to test?  If not, can you find someone
who can?  Maybe Kritphong?  If not, the whole thing becomes a rather
pointless endeavour.

Having poured over the code for the better part of a weekend, I'd say
the transmission of strings should not be treated as the transmission
of an array (of characters).  It looks to me like the sanei_w_array()
code can be used fine when transferring the constraint member of a
SANE_Option_Descriptor but I am not convinced it is the right thing
to use when *getting* an option's SANE_String value.  When getting an
option's SANE_String value, the code *should* allocate a buffer big
enough to hold the *largest* possible string even if the net backend is
sending a (much) smaller string.  The size of the largest possible
string is given by the SANE_Option_Descriptor's size member for options
that have an option value type of SANE_TYPE_STRING.

# Please refer to the API spec for the details.

Based on a quick look at your patch, you may be heading in the right
direction but I'd really like to see this confirmed by:

 - tests indicating that saned works (as in you can get/set options
   with string values and scan without trouble)
 - packet captures that show no uninitialized bits of memory go fly
   over the wire (we know that the third party hpaio backend will
   trigger these from Kritphong's bug report so that would be a good
   backend to test with).
 - (optionally but very much recommended) an indication that there
   are no memory issues in saned (think valgrind logs)

That's quite a bit of work and testing for which I unfortunately do not
have the time right now.  If you do, then, by all means, go ahead and
whip up a real fix to replace my somewhat iffy patch.

Hope this helps,
--
Olaf Meeuwissen, LPIC-2FSF Associate Member since 2004-01-27
 GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9
 Support Free Softwarehttps://my.fsf.org/donate
 Join the Free Software Foundation  https://my.fsf.org/join

Bug#854804: [sane-devel] Bug#854804: saned: SANE_NET_CONTROL_OPTION response packet may contain memory contents of the server

[Zdenek Dohnal]

On 03/05/2017 10:40 AM, Olaf Meeuwissen wrote:
> Hi Zdenek,
>
> Zdenek Dohnal writes:
>
>> I tried to enhanced Olaf's patch and I posted it here:
>>
>> https://paste.fedoraproject.org/paste/qssgq4s0Vtqw6R5wkDWoEV5M1UNdIGYhyRLivL9gydE=
>>
>> Were my thoughts right and will it solve this issue?
> Thinking you just "backported" the patch (it applies with a fuzz but
> otherwise cleanly against 1.0.25) and removed the comments, I almost
> overlook your code change!
>
> I think it's my FIXME that misled you but you should *not* substract
> req.value_size.  Doing so is worse than what my code does because your
> code would substract too much, quite possibly making w->allocated_memory
> negative.  My code runs the risk of not substracting enough.
>
> In sanei/sanei_wire.c bytes are allocated to hold req.value based on a
> number provided by the network protocol.  This number is large enough to
> hold req.value plus terminating NUL and not larger than req.value_size.
>
> # In the original issue, req.value_size is 1024 and req.value = '\0'.
> # The code in sanei/sanei_wire.c allocates *1* byte.
>
> What the code in sanei/sanei_wire.c should do is allocate space for
> req.value_size bytes (it can't because where the allocation happens this
> information is not available).  My patch frees the incorrectly allocate
> memory and allocates a chunk that big enough.  It does that in saned.c
> to minimize its impact.
>
> # The sanei/sanei_wire.c code is used by saned *and* the net backend for
> # I/O in both directions.  To complicate matters, the code is meant to
> # transfer arrays and "abused" to transfer strings as if they are arrays
> # of characters.  My patch only affects saned's read logic.
> # A better patch would actually fix the issue(s) in sanei/sanei_wire.c.
>
> Doing this in saned.c though means that I no longer have access to the
> number provided by the network protocol.  I have to rely on the string
> length which may be less.  Hence my FIXME comment.
>
> # I was thinking about scenarios where backends might stuff a string in
> # a slightly larger buffer than strictly necessary and send the whole
> # buffer.
>
> Hope this clarifies a bit,
> --
> Olaf Meeuwissen, LPIC-2FSF Associate Member since 2004-01-27
>  GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9
>  Support Free Softwarehttps://my.fsf.org/donate
>  Join the Free Software Foundation  https://my.fsf.org/join
Thank you so much for explanation, Olaf. I did not notice that fact
about req.value_size. So what about fetching string length from
sanei_w_array function by parameters sent by reference? Is it acceptable
to change number and type of parameters of functions? I created patch
proposal:

https://paste.fedoraproject.org/paste/KVJpdlIAMcxiovnYF4dhbV5M1UNdIGYhyRLivL9gydE=

It is probably not final version, but I hope I demonstrated my idea. It
was compiled without error.

-- 
Zdenek Dohnal
Associate Software Engineer
Brno, Purkyňova 99, Czech Republic
RED HAT | TRIED. TESTED. TRUSTED.

Every telecommunications Company in the Fortune Global 500 relies on Red Hat.

Find out why at Trusted | Red Hat




signature.asc
Description: OpenPGP digital signature

Bug#854804: [sane-devel] Bug#854804: saned: SANE_NET_CONTROL_OPTION response packet may contain memory contents of the server

[Olaf Meeuwissen]

Hi Zdenek,

Zdenek Dohnal writes:

> I tried to enhanced Olaf's patch and I posted it here:
>
> https://paste.fedoraproject.org/paste/qssgq4s0Vtqw6R5wkDWoEV5M1UNdIGYhyRLivL9gydE=
>
> Were my thoughts right and will it solve this issue?

Thinking you just "backported" the patch (it applies with a fuzz but
otherwise cleanly against 1.0.25) and removed the comments, I almost
overlook your code change!

I think it's my FIXME that misled you but you should *not* substract
req.value_size.  Doing so is worse than what my code does because your
code would substract too much, quite possibly making w->allocated_memory
negative.  My code runs the risk of not substracting enough.

In sanei/sanei_wire.c bytes are allocated to hold req.value based on a
number provided by the network protocol.  This number is large enough to
hold req.value plus terminating NUL and not larger than req.value_size.

# In the original issue, req.value_size is 1024 and req.value = '\0'.
# The code in sanei/sanei_wire.c allocates *1* byte.

What the code in sanei/sanei_wire.c should do is allocate space for
req.value_size bytes (it can't because where the allocation happens this
information is not available).  My patch frees the incorrectly allocate
memory and allocates a chunk that big enough.  It does that in saned.c
to minimize its impact.

# The sanei/sanei_wire.c code is used by saned *and* the net backend for
# I/O in both directions.  To complicate matters, the code is meant to
# transfer arrays and "abused" to transfer strings as if they are arrays
# of characters.  My patch only affects saned's read logic.
# A better patch would actually fix the issue(s) in sanei/sanei_wire.c.

Doing this in saned.c though means that I no longer have access to the
number provided by the network protocol.  I have to rely on the string
length which may be less.  Hence my FIXME comment.

# I was thinking about scenarios where backends might stuff a string in
# a slightly larger buffer than strictly necessary and send the whole
# buffer.

Hope this clarifies a bit,
--
Olaf Meeuwissen, LPIC-2FSF Associate Member since 2004-01-27
 GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9
 Support Free Softwarehttps://my.fsf.org/donate
 Join the Free Software Foundation  https://my.fsf.org/join

Bug#854804: [sane-devel] Bug#854804: saned: SANE_NET_CONTROL_OPTION response packet may contain memory contents of the server

[Zdenek Dohnal]

Hi,

I tried to enhanced Olaf's patch and I posted it here:

https://paste.fedoraproject.org/paste/qssgq4s0Vtqw6R5wkDWoEV5M1UNdIGYhyRLivL9gydE=

Were my thoughts right and will it solve this issue?

Thank you in advance.

-- 
Zdenek Dohnal
Associate Software Engineer
Brno, Purkyňova 99, Czech Republic
RED HAT | TRIED. TESTED. TRUSTED.

Every telecommunications Company in the Fortune Global 500 relies on Red Hat.

Find out why at Trusted | Red Hat




signature.asc
Description: OpenPGP digital signature

Bug#854804: [sane-devel] Bug#854804: saned: SANE_NET_CONTROL_OPTION response packet may contain memory contents of the server

[Olaf Meeuwissen]

Hi Kritphong,

Kritphong Mongkhonvanit writes:

> Hi Olaf,
>
>
> On 02/19/2017 02:53 PM, Olaf Meeuwissen wrote:
>> Attached is a minimal hack/patch that *tries* to fix it.  I have only
>> checked that it compiles.  Could you take a look at whether it fixes
>> the issue and does not break saned?
> Thank you for your patch. I performed some basic tests and it seems to
> fix the issue for me. It doesn't break saned as far as I can tell.

That's good news.

@sane-devel> If some of you could review the patch[0] and do some
 testing that would be appreciated.

 [0] 
http://lists.alioth.debian.org/pipermail/sane-devel/2017-February/035054.html

If someone is willing to pull saned through valgrind and post the
results to the mailing list (don't spam the Debian BTS with this,
please), that'd be appreciated as well.
# I'm a just a wee bit worried there is more amiss with saned.

Alternatively, open a tracker issue[1] and assign it to me.

 [1] https://alioth.debian.org/tracker/?func=add_id=30186=410366

Hope this helps,
--
Olaf Meeuwissen, LPIC-2FSF Associate Member since 2004-01-27
 GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9
 Support Free Softwarehttps://my.fsf.org/donate
 Join the Free Software Foundation  https://my.fsf.org/join

Bug#854804: [sane-devel] Bug#854804: saned: SANE_NET_CONTROL_OPTION response packet may contain memory contents of the server

[Kritphong Mongkhonvanit]

Hi Olaf,


On 02/19/2017 02:53 PM, Olaf Meeuwissen wrote:
> Attached is a minimal hack/patch that *tries* to fix it.  I have only
> checked that it compiles.  Could you take a look at whether it fixes
> the issue and does not break saned?
Thank you for your patch. I performed some basic tests and it seems to
fix the issue for me. It doesn't break saned as far as I can tell.

Bug#854804: [sane-devel] Bug#854804: saned: SANE_NET_CONTROL_OPTION response packet may contain memory contents of the server

[Olaf Meeuwissen]

Hi Kritphong,

Kritphong Mongkhonvanit writes:

> On 02/14/2017 09:04 PM, Olaf Meeuwissen wrote:
>> Could you run
>>
>>   SANE_DEBUG_SANEI_WIRE=128 saned -d128 2> saned.log
>>
>> reproduce and provide the saned.log (compressed if big)?
> The requested log is attached.

Thanks!!

I didn't write the code but, if my analysis is correct, it is actually
worse than sending server memory content over the wire.  It looks like
saned is clobbering memory, i.e. it's writing past the end of allocated
memory, as well.

According to your log (at line 4007), the saned process gets its first
SANE_NET_CONTROL_OPTION request.  That request tries to fetch the value
of the 8th option (compression) which is a string value that can be up
to 1024 (0x400) bytes long.  The request also sends a value with this
request, a NUL-terminated 1-byte long empty string.

# Code line references against f450049b.

At this point we are around line 4045 of the log.  Now let's switch to
the code.  The incoming request is handled in the case statement on line
1979 of frontend/saned.c.  The sanei_w_control_option_req() call has
taken care of the incoming request and the req structure now contains

  req.handle = 0;
  req.option = 8;  // 'compression'
  req.action = 0;  // SANE_ACTION_GET_VALUE
  req.value_type = 3;  // SANE_TYPE_STRING
  req.value_size = 1024;
  req.value  = "\0";

Most importantly, req.value was allocated as a *1*-byte buffer.  This
happens in the if-block starting at line 204 in sanei/sanei_wire.c.
Note that the `len` is passed back up via `len_ptr` but that that value
does *not* make it back to req.value_size because the w_option_value()
call in sanei_w_control_option_req() passes by value, not by reference.

This means that sane_control_option() on line 1999 in frontend/saned.c
happily passes a 1-byte buffer to the backend.  The backend assumes that
it can store up to 1024 bytes in that buffer and writes a NUL-terminated
five byte "JPEG" string into the 1-byte buffer.  Oops!

On line 2003 of frontend/saned.c the reply.value_size is set to the
value fo req.value_size (still 1024) and sanei_w_reply gets a reply
struct that:

 - has a pointer to a 1-byte block of memory
 - which holds a five byte string value
 - that is sent back as a 1024 buffer

Ouch!

This code has been around since the summer of 1999.  Seeing that we have
not had anyone complain about this before, please check my analysis with
care.  I have only "eyeballed" the code.  I have not tried to reproduce
or run things in a debugger or anything.

Attached is a minimal hack/patch that *tries* to fix it.  I have only
checked that it compiles.  Could you take a look at whether it fixes
the issue and does not break saned?

Hope this helps,
--
Olaf Meeuwissen, LPIC-2FSF Associate Member since 2004-01-27
 GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9
 Support Free Softwarehttps://my.fsf.org/donate
 Join the Free Software Foundation  https://my.fsf.org/join
>From 46bab8cea5000b363ca4eb360e635285feb14ac7 Mon Sep 17 00:00:00 2001
From: Olaf Meeuwissen 
Date: Sun, 19 Feb 2017 16:45:45 +0900
Subject: [PATCH] Address memory corruption and information leakage.

---
 frontend/saned.c | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/frontend/saned.c b/frontend/saned.c
index 0aba1755..8c4fa13a 100644
--- a/frontend/saned.c
+++ b/frontend/saned.c
@@ -1992,6 +1992,32 @@ process_request (Wire * w)
 	return 1;
 	  }
 
+/* Debian BTS #853804 */
+if (w->direction == WIRE_DECODE
+&& req.value_type == SANE_TYPE_STRING
+&& req.action == SANE_ACTION_GET_VALUE)
+  {
+if (req.value)
+  {
+/* FIXME: If req.value contained embedded NUL
+ *characters, this is wrong.
+ */
+w->allocated_memory -= (1 + strlen (req.value));
+free (req.value);
+  }
+req.value = malloc (req.value_size);
+if (!req.value)
+  {
+w->status = ENOMEM;
+DBG (DBG_ERR,
+ "process_request: (control_option) "
+ "h=%d (%s)\n", req.handle, strerror (w->status));
+return 1;
+  }
+memset (req.value, 0, req.value_size);
+w->allocated_memory += req.value_size;
+  }
+
 	can_authorize = 1;
 
 	memset (, 0, sizeof (reply));	/* avoid leaking bits */
-- 
2.11.0

Bug#854804: [sane-devel] Bug#854804: saned: SANE_NET_CONTROL_OPTION response packet may contain memory contents of the server

[Olaf Meeuwissen]

Hi Kritphong, Jörg,

Kritphong Mongkhonvanit writes:

> Hello Jörg,
>
> On 02/12/2017 02:43 PM, Jörg Frings-Fürst wrote:
>
>> [snip BTS control commands]
>>
>> Hello Kritphong,
>>
>> Am Sonntag, den 12.02.2017, 00:16 +0700 schrieb Kritphong
>> Mongkhonvanit:
>>> [snip BTS control commands]
>>>
>>> On Sat, Feb 11, 2017 at 11:54 AM, Jörg Frings-Fürst 
>>>  wrote:
>> [...]
 Am Freitag, den 10.02.2017, 10:33 -0500 schrieb Kritphong
 Mongkhonvanit:
>> [...]
  Dear Maintainer,

  When saned received a SANE_NET_CONTROL_OPTION packet with value_type ==
  SANE_TYPE_STRING and value_size larger than the actual length of the
  requested string, the response packet from the server contains a string
  object as long as value_size in the request. The bytes following the
  actual string appears to contain memory contents from the server.

 Please let me explain:

 You have found one or more parts in the code where a string with an
 incorrect value_size is transferred? Then please tell us where.
>>>
>>> I found that the transferred string in the value field of
>>> SANE_NET_CONTROL_OPTION response packet is always the same size as
>>> the one requested, even if the actual string is shorter. I assume
>>> that this is intentional since the string is
>>> NULL-terminated. However, the part beyond the NULL-terminator
>>> appears to be uninitialized memory from the server, which can
>>> potentially contain sensitive information. I have yet to locate
>>> where in SANE's source code this is happening, but I am able to see
>>> the uninitialized memory in Wireshark, which suggests that it
>>> actually comes from the server rather than from my machine.
>>>
>> [...]
>>
>> At a short code search I have found a point of use in net.c.
>>
>> The authors are aware that the strings can be shorter than the
>> transferred size. You have written the appropriate code that ensures
>> that the strings only use the part until the final NULL.

That's the `case SANE_TYPE_STRING` in backend/net.c#1753.

>> Furthermore, before using the structure, it is overwritten with NULL.

That's the `memset` in backend/net.c#1767, right?  Or are you referring
to frontend/saned.c#1997?

>> At this point I don't see any security hole. So I set the severity to
>> important. In the future, I will close the bug, unless you create new
>> threats.
>>
> I do realize that there is a part where the memory was zeroed in net.c.
> However, there must be somewhere else where uninitialized memory was
> copied and sent since the bytes following the string are not exclusively
> zeros.
>
> Please take a look at the decoded SANE_NET_CONTROL_OPTION response

If it's in the *response*, then it comes from frontend/saned.c, not the
backend/net.c code.  I've been chasing the code up and down and am by
now fairly sure it is caused somewhere in the sanei/sanei_wire.c code.
I just don't see where.

Could you run

  SANE_DEBUG_SANEI_WIRE=128 saned -d128 2> saned.log

reproduce and provide the saned.log (compressed if big)?
# Running saned through valgrind may also turn up hints, BTW.

> packet I captured in Wireshark below.
>
> JPEGSignerIdentifierdigestAlgori
> thm..l.=...@@...
> X...8...AlgorithmIde
> ntifier.signedAttrs.
> .`..x...
> `...SignedAttributes
> `...X...0...
> signatureAlgorithm..
> .p.@...8...X
> g.AlgorithmIdentifier.signature.
> .;...@..
> ..unsignedAttrs.
> ../#...`..X...p.
> ..8...h...SignedAttribut
> es
>
> Here's an excerpt of the corresponding hex stream. I omitted the part
> after the string since it looks like it may contain sensitive
> information.
>
>   0003 0400 0400 4a504547 00 (omitted)
>
> As you can see, the string "JPEG" is NULL-terminated at byte 25, and the
> bytes after that are clearly not all zeroes. Both value_size (the 4th
> word) and the length of the string object (the 5th word) are set to
> 0x400, so they must have been sent by saned as a part of the string
> object.

Hope this helps,
--
Olaf Meeuwissen, LPIC-2FSF Associate Member since 2004-01-27
 GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9
 Support Free Software