[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.

[Olaf Meeuwissen]

Hi Zdenek,

Post by Zdenek Dohnal
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-2 FSF Associate Member since 2004-01-27
GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13 F43E B8A4 A88A F84A 2DD9
Support Free Software https://my.fsf.org/donate
Join the Free Software Foundation https://my.fsf.org/join

[Zdenek Dohnal]

Post by Olaf Meeuwissen
Hi Zdenek,

Post by Zdenek Dohnal
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-2 FSF Associate Member since 2004-01-27
GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13 F43E B8A4 A88A F84A 2DD9
Support Free Software https://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.

[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 :-)

Post by Zdenek Dohnal

Post by Olaf Meeuwissen
Hi Zdenek,

Post by Zdenek Dohnal
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
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-2 FSF Associate Member since 2004-01-27
GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13 F43E B8A4 A88A F84A 2DD9
Support Free Software https://my.fsf.org/donate
Join the Free Software Foundation https://my.fsf.org/join