Re: [sane-devel] 39ceeae6 breaks md5 auth

[Olaf Meeuwissen]

Hi James,

James Ring writes:

> Hi Olaf,
>
> On Tue, Jan 2, 2018 at 11:15 PM, Olaf Meeuwissen
>  wrote:
>> Hi James,
>>
>> Thanks for the report.
>>
>> James Ring writes:
>>
>>> Confirmed that with the offending patch, md5.c produces incorrect
>>> digests for known input/output pairs. We should roll it back.
>>>
>>> Also I couldn't reproduce (with gcc 7.2.0) the compiler warning that
>>> the original change was supposed to fix.
>>
>> Hmm, neither can I.  In neither the debian-8-mini nor debian-9-mini CI
>> environments.  But you can still see it in the CI logs for the last
>> pipeline that ran before the offending commit.
>>
>>   https://gitlab.com/sane-project/backends/pipelines/4150861
>>
>> Only the fedora-24-clang log doesn't have it.
>
> Unrelated note: if I read those logs correctly, the CI doesn't run
> `make check`, which maybe they should. It would also be nice to have a
> test case that exercises authentication, though I suspect it is an
> infrequently used feature. I tried adding one, but I couldn't figure
> out how to update the makefiles to include my test.

A `make check` requires /dev/usb to be present in the Docker container
used for the builds in order to pass.  While I have no problem doing
that locally, I haven't tried (or don't recall doing so ;-) on the
shared GitLab.com CI runners.  The VMs that these run on may not even
*have* /dev/usb.

A for an auth test case, a patch with the test is welcome.  I can update
the automake files to integrate the test in the `make check` target.

>>> On Tue, Jan 2, 2018 at 9:57 AM, James Ring  wrote:
 [...snip...]

 Reverting that commit restores the functionality. I haven't figured
 out what the problem is from a cursory inspection of the code, I'll
 continue staring at it.
>>
>> I was about to revert the commit but looking at it now I'm wondering
>> what I was thinking when I committed that :-(  Changing the pointer
>> type to something of a different size *obviously* screws up the array
>> indexing!
>>
>> I've cooked up a fix for that (based on e895ee55).  Could you give the
>> attached patch a try?
>
> Yes, this works! I can't help wondering if it might be better to
> introduce an external dependency on, e.g. libxcrypt or libssl to
> implement md5? That way the sane project is not maintaining an md5
> fork.

Thanks for the testing.  Patch has been pushed.

As for the alternative libraries, thanks for the suggestion.  I noticed
that there is also libcrypt which comes with glibc.  Not sure if there
is anything similar for musl or any of the other OSs that are nominally
supported by sane-backends.  At present, it is less work to maintain the
included code than switch to some "standard" library.

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

-- 
sane-devel mailing list: sane-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
 to sane-devel-requ...@lists.alioth.debian.org

Re: [sane-devel] 39ceeae6 breaks md5 auth

[James Ring]

Hi Olaf,

On Tue, Jan 2, 2018 at 11:15 PM, Olaf Meeuwissen
 wrote:
> Hi James,
>
> Thanks for the report.
>
> James Ring writes:
>
>> Confirmed that with the offending patch, md5.c produces incorrect
>> digests for known input/output pairs. We should roll it back.
>>
>> Also I couldn't reproduce (with gcc 7.2.0) the compiler warning that
>> the original change was supposed to fix.
>
> Hmm, neither can I.  In neither the debian-8-mini nor debian-9-mini CI
> environments.  But you can still see it in the CI logs for the last
> pipeline that ran before the offending commit.
>
>   https://gitlab.com/sane-project/backends/pipelines/4150861
>
> Only the fedora-24-clang log doesn't have it.

Unrelated note: if I read those logs correctly, the CI doesn't run
`make check`, which maybe they should. It would also be nice to have a
test case that exercises authentication, though I suspect it is an
infrequently used feature. I tried adding one, but I couldn't figure
out how to update the makefiles to include my test.

>> On Tue, Jan 2, 2018 at 9:57 AM, James Ring  wrote:
>>> [...snip...]
>>>
>>> Reverting that commit restores the functionality. I haven't figured
>>> out what the problem is from a cursory inspection of the code, I'll
>>> continue staring at it.
>
> I was about to revert the commit but looking at it now I'm wondering
> what I was thinking when I committed that :-(  Changing the pointer
> type to something of a different size *obviously* screws up the array
> indexing!
>
> I've cooked up a fix for that (based on e895ee55).  Could you give the
> attached patch a try?

Yes, this works! I can't help wondering if it might be better to
introduce an external dependency on, e.g. libxcrypt or libssl to
implement md5? That way the sane project is not maintaining an md5
fork.

Thank you for your help!

James

-- 
sane-devel mailing list: sane-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
 to sane-devel-requ...@lists.alioth.debian.org

Re: [sane-devel] 39ceeae6 breaks md5 auth

[Olaf Meeuwissen]

Hi James,

Thanks for the report.

James Ring writes:

> Confirmed that with the offending patch, md5.c produces incorrect
> digests for known input/output pairs. We should roll it back.
>
> Also I couldn't reproduce (with gcc 7.2.0) the compiler warning that
> the original change was supposed to fix.

Hmm, neither can I.  In neither the debian-8-mini nor debian-9-mini CI
environments.  But you can still see it in the CI logs for the last
pipeline that ran before the offending commit.

  https://gitlab.com/sane-project/backends/pipelines/4150861

Only the fedora-24-clang log doesn't have it.

> On Tue, Jan 2, 2018 at 9:57 AM, James Ring  wrote:
>> [...snip...]
>>
>> Reverting that commit restores the functionality. I haven't figured
>> out what the problem is from a cursory inspection of the code, I'll
>> continue staring at it.

I was about to revert the commit but looking at it now I'm wondering
what I was thinking when I committed that :-(  Changing the pointer
type to something of a different size *obviously* screws up the array
indexing!

I've cooked up a fix for that (based on e895ee55).  Could you give the
attached patch a try?

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 3651d8c6cd943a1d046e2625bb7bf2eca6f91c42 Mon Sep 17 00:00:00 2001
From: Olaf Meeuwissen 
Date: Wed, 3 Jan 2018 16:13:16 +0900
Subject: [PATCH] Fix array indexing.

This fixes a glaring oversight in 39ceeae6.  Thanks to James Ring for
reporting this.
---
 lib/md5.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/md5.c b/lib/md5.c
index 72b36f35..923a17c7 100644
--- a/lib/md5.c
+++ b/lib/md5.c
@@ -123,6 +123,7 @@ md5_finish_ctx (struct md5_ctx *ctx, void *resbuf)
   /* Take yet unprocessed bytes into account.  */
   md5_uint32 bytes = ctx->buflen;
   size_t pad;
+  size_t offset;
 
   /* Now count remaining bytes.  */
   ctx->total[0] += bytes;
@@ -133,9 +134,11 @@ md5_finish_ctx (struct md5_ctx *ctx, void *resbuf)
   memcpy (>buffer[bytes], fillbuf, pad);
 
   /* Put the 64-bit file length in *bits* at the end of the buffer.  */
-  ((md5_uint32 *) ctx->buffer)[bytes + pad] = SWAP (ctx->total[0] << 3);
-  ((md5_uint32 *) ctx->buffer)[bytes + pad + 4] = SWAP ((ctx->total[1] << 3) |
-			(ctx->total[0] >> 29));
+  offset = (bytes + pad) / sizeof (md5_uint32);
+  ((md5_uint32 *) ctx->buffer)[offset] = SWAP (ctx->total[0] << 3);
+  offset = (bytes + pad + 4) / sizeof (md5_uint32);
+  ((md5_uint32 *) ctx->buffer)[offset] = SWAP ((ctx->total[1] << 3) |
+	   (ctx->total[0] >> 29));
 
   /* Process last bytes.  */
   md5_process_block (ctx->buffer, bytes + pad + 8, ctx);
-- 
2.11.0

-- 
sane-devel mailing list: sane-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
 to sane-devel-requ...@lists.alioth.debian.org

Re: [sane-devel] 39ceeae6 breaks md5 auth

[James Ring]

Confirmed that with the offending patch, md5.c produces incorrect
digests for known input/output pairs. We should roll it back.

Also I couldn't reproduce (with gcc 7.2.0) the compiler warning that
the original change was supposed to fix.

On Tue, Jan 2, 2018 at 9:57 AM, James Ring  wrote:
> Hi Olaf,
>
> I was trying to find out why authentication to a local test device
> does not work with my frontend on Ubuntu 17.10.
>
> $ scanimage -d net:localhost:test
> scanimage: open of device net:localhost:test failed: Access to
> resource has been denied
>
> Both saned.users and $HOME/.sane/sane.pass contain the following line:
>
> testuser:goodpass:net:localhost:test
>
> I tried building sane 1.0.27 from source and could reproduce the
> error. 1.0.24 built from source did not show the issue. Bisecting
> reveals that the culprit is
>
> https://anonscm.debian.org/git/sane/sane-backends.git/commit/?id=39ceeae616a2e1638c2760d4364adcaa210a413b
>
> Reverting that commit restores the functionality. I haven't figured
> out what the problem is from a cursory inspection of the code, I'll
> continue staring at it.
>
> Thanks,
> James

-- 
sane-devel mailing list: sane-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
 to sane-devel-requ...@lists.alioth.debian.org

[sane-devel] 39ceeae6 breaks md5 auth

[James Ring]

Hi Olaf,

I was trying to find out why authentication to a local test device
does not work with my frontend on Ubuntu 17.10.

$ scanimage -d net:localhost:test
scanimage: open of device net:localhost:test failed: Access to
resource has been denied

Both saned.users and $HOME/.sane/sane.pass contain the following line:

testuser:goodpass:net:localhost:test

I tried building sane 1.0.27 from source and could reproduce the
error. 1.0.24 built from source did not show the issue. Bisecting
reveals that the culprit is

https://anonscm.debian.org/git/sane/sane-backends.git/commit/?id=39ceeae616a2e1638c2760d4364adcaa210a413b

Reverting that commit restores the functionality. I haven't figured
out what the problem is from a cursory inspection of the code, I'll
continue staring at it.

Thanks,
James

-- 
sane-devel mailing list: sane-devel@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
 to sane-devel-requ...@lists.alioth.debian.org