Mailing List Archive

On Wed, 6 May 2020, Peter wrote:

> compile_link is documented to allow the path to the socket in vlink to
> be overridden with the MINIVEND_SOCKET environment variable, but this
> functionality is non-existent. Since at the moment I'm attempting to
> package Interchange 5.12.0 for CentOS 8 I'm thinking that this
> functionality would be usefull so that I can simply build vlink for the
> default socket location and if someone wants to change it they can just
> set MINIVEND_SOCKET rather than having to re-compile vlink with the new
> location. I've come up with the following patch which should do that (I
> haven't tested it yet), and also eliminates a vulnerable strcpy in the
> process:

Looks good to me (also not testing, just reading).

We should probably make the same change in vlink.pl, I guess.

Jon



--
Jon Jensen
End Point Corporation
https://www.endpoint.com/
_______________________________________________
interchange-users mailing list
interchange-users@interchangecommerce.org
https://www.interchangecommerce.org/mailman/listinfo/interchange-users

Hi Peter,

Thanks for the patch.

> sa.sun_family = AF_UNIX;
> - strcpy(sa.sun_path, LINK_FILE);
> + strncpy(sa.sun_path, lsocket, sizeof(sa.sun_path) - 1);

A couple of questions: is the sa struct guaranteed to be zero’d, so we don’t need to worry about a NULL terminator (i.e., why there’s a -1 in sizeof call)? Also, if strlen(lsocket) > sizeof(sa.sun_path), a truncated copy would end up being bunko, since it won’t refer to an actual valid path; would it be better to just check if strlen(lsocket) > sizeof(sa.sun_path) -1 and error out if so?

Best,

David
--
David Christensen
Senior Software and Database Engineer
End Point Corporation
david@endpoint.com
785-727-1171

On Tue, 5 May 2020, David Christensen wrote:

>> + strncpy(sa.sun_path, lsocket, sizeof(sa.sun_path) - 1);
>
> A couple of questions: is the sa struct guaranteed to be zero’d, so we
> don’t need to worry about a NULL terminator (i.e., why there’s a -1 in
> sizeof call)?

Using 1 byte smaller than sizeof, strncpy() guarantees null termination,
because it clears the rest of the buffer, so it will have at least one
null at the end.

> Also, if strlen(lsocket) > sizeof(sa.sun_path), a truncated copy would
> end up being bunko, since it won’t refer to an actual valid path; would
> it be better to just check if strlen(lsocket) > sizeof(sa.sun_path) -1
> and error out if so?

Great point. No reason to proceed if the filename will be truncated.

Jon



--
Jon Jensen
End Point Corporation
https://www.endpoint.com/

On 6/05/20 4:58 am, Jon Jensen wrote:
> On Tue, 5 May 2020, David Christensen wrote:
>
>>> +  strncpy(sa.sun_path, lsocket, sizeof(sa.sun_path) - 1);
>>
>> A couple of questions: is the sa struct guaranteed to be zero’d, so we
>> don’t need to worry about a NULL terminator (i.e., why there’s a -1 in
>> sizeof call)?
>
> Using 1 byte smaller than sizeof, strncpy() guarantees null termination,
> because it clears the rest of the buffer, so it will have at least one
> null at the end.

Exactly, and I took the code almost verbatim from unix(7):
http://man7.org/linux/man-pages/man7/unix.7.html

>> Also, if strlen(lsocket) > sizeof(sa.sun_path), a truncated copy would
>> end up being bunko, since it won’t refer to an actual valid path;
>> would it be better to just check if strlen(lsocket) >
>> sizeof(sa.sun_path) -1 and error out if so?
>
> Great point. No reason to proceed if the filename will be truncated.

I agree, but I think we should do both, even though only one or the
other is needed to prevent a buffer overflow both just makes doubly
safe, and I tend to prefer to get rid of strcpy in favor of strncpy
where I see it.

BTW I just stayed up nearly all night rewriting the spec file. I think
it will be nice to have some decent RPM packages to go along with the
5.12.0 release which I'm happy to host in GhettoForge. I'll be testing
tomorrow.


Peter
_______________________________________________
interchange-users mailing list
interchange-users@interchangecommerce.org
https://www.interchangecommerce.org/mailman/listinfo/interchange-users

On 6/05/20 5:30 am, Peter wrote:
>>> Also, if strlen(lsocket) > sizeof(sa.sun_path), a truncated copy
>>> would end up being bunko, since it won’t refer to an actual valid
>>> path; would it be better to just check if strlen(lsocket) >
>>> sizeof(sa.sun_path) -1 and error out if so?
>>
>> Great point. No reason to proceed if the filename will be truncated.
>
> I agree, but I think we should do both, even though only one or the
> other is needed to prevent a buffer overflow both just makes doubly
> safe, and I tend to prefer to get rid of strcpy in favor of strncpy
> where I see it.

This should patch both vlink.c and vlink.pl. I still haven't tested:

--- interchange-5.12.0-rc1/dist/src/vlink.c.orig 2020-05-05
23:09:11.408139523 +1200
+++ interchange-5.12.0-rc1/dist/src/vlink.c 2020-05-06
05:58:00.769192141 +1200
@@ -37,6 +37,7 @@
#include <sys/socket.h>
#include <sys/un.h>
#include <unistd.h>
+#include <asm-generic/errno.h>

#ifndef ENVIRON_DECLARED
extern char** environ;
@@ -140,12 +141,20 @@
int i;
int e;
int r;
+ char *lsocket;
uid_t euid;
gid_t egid;

+ lsocket = getenv("MINIVEND_SOCKET");
+ if(lsocket == NULL) {
+ lsocket = LINK_FILE;
+ }
+
+ if(strlen(lsocket) > sizeof(sa.sun_path) - 1)
+ die(ENAMETOOLONG, "Socket file name too long");

sa.sun_family = AF_UNIX;
- strcpy(sa.sun_path, LINK_FILE);
+ strncpy(sa.sun_path, lsocket, sizeof(sa.sun_path) - 1);
#ifdef offsetof
size = (offsetof (struct sockaddr_un, sun_path) + strlen
(sa.sun_path) + 1);
#else
--- interchange-5.12.0-rc1/dist/src/vlink.pl.orig 2020-02-28
04:07:50.000000000 +1300
+++ interchange-5.12.0-rc1/dist/src/vlink.pl 2020-05-06
05:52:33.648898727 +1200
@@ -24,7 +24,7 @@
require 5.014_001;
use strict;
use Socket;
-my $LINK_FILE = '~@~INSTALLARCHLIB~@~/etc/socket';
+my $LINK_FILE = $ENV{MINIVEND_SOCKET} ||
'~@~INSTALLARCHLIB~@~/etc/socket';
#my $LINK_FILE = '~_~LINK_FILE~_~';
my $LINK_TIMEOUT = 30;
#my $LINK_TIMEOUT = ~_~LINK_TIMEOUT~_~;


Peter
_______________________________________________
interchange-users mailing list
interchange-users@interchangecommerce.org
https://www.interchangecommerce.org/mailman/listinfo/interchange-users

>> On May 5, 2020, at 1:03 PM, Peter Ajamian <peter@pajamian.dhs.org> wrote:
>>
>> On 6/05/20 5:30 am, Peter wrote:
>>>>> Also, if strlen(lsocket) > sizeof(sa.sun_path), a truncated copy would end up being bunko, since it won’t refer to an actual valid path; would it be better to just check if strlen(lsocket) > sizeof(sa.sun_path) -1 and error out if so?
>>>>
>>>> Great point. No reason to proceed if the filename will be truncated.
>>> I agree, but I think we should do both, even though only one or the other is needed to prevent a buffer overflow both just makes doubly safe, and I tend to prefer to get rid of strcpy in favor of strncpy where I see it.
>>
>> This should patch both vlink.c and vlink.pl. I still haven't tested:

Looks good to me, thanks.
--
David Christensen
Senior Software and Database Engineer
End Point Corporation
david@endpoint.com
785-727-1171

On Wed, 6 May 2020, Peter Ajamian wrote:

> On 6/05/20 5:30 am, Peter wrote:
>>>> Also, if strlen(lsocket) > sizeof(sa.sun_path), a truncated copy would
>>>> end up being bunko, since it won’t refer to an actual valid path; would
>>>> it be better to just check if strlen(lsocket) > sizeof(sa.sun_path) -1
>>>> and error out if so?
>>>
>>> Great point. No reason to proceed if the filename will be truncated.
>>
>> I agree, but I think we should do both, even though only one or the other
>> is needed to prevent a buffer overflow both just makes doubly safe, and I
>> tend to prefer to get rid of strcpy in favor of strncpy where I see it.
>
> This should patch both vlink.c and vlink.pl. I still haven't tested:
[snip]

Peter,

I just wanted to check in on your vlink.c and vlink.pl updates to respect
the MINIVEND_SOCKET environment variable.

Did you run into any snags? Will you be able to commit those changes to
Git soon?

Thanks,
Jon



--
Jon Jensen
End Point Corporation
https://www.endpoint.com/

On 26/06/20 4:18 am, Jon Jensen wrote:
> Peter,
>
> I just wanted to check in on your vlink.c and vlink.pl updates to
> respect the MINIVEND_SOCKET environment variable.

Unfortuntately I haven't had time to test them yet I'v ejust been too
busy with other stuff. I'll try and get that tested today or tomorrow,
though. Incidentally I do have the patch included in my rpm build:

http://mirror.ghettoforge.org/distributions/gf/el/8/testing/x86_64/

> Did you run into any snags? Will you be able to commit those changes to
> Git soon?

I don't seem to have write access since we switched to github.


Peter
_______________________________________________
interchange-users mailing list
interchange-users@interchangecommerce.org
https://www.interchangecommerce.org/mailman/listinfo/interchange-users

On 26/06/20 7:04 am, Peter wrote:
> On 26/06/20 4:18 am, Jon Jensen wrote:
>> Peter,
>>
>> I just wanted to check in on your vlink.c and vlink.pl updates to
>> respect the MINIVEND_SOCKET environment variable.
>
> Unfortuntately I haven't had time to test them yet I'v ejust been too
> busy with other stuff.  I'll try and get that tested today or tomorrow,
> though.

...tested, the vlink.c patch works well. The vlink.pl patch has issues
because vlink.pl is running with -T and $ENV{MINIVEND_SOCKET} is
tainted. If I untaint it then it works as well. I'll update the patch
and then push it out tomorrow.


Peter
_______________________________________________
interchange-users mailing list
interchange-users@interchangecommerce.org
https://www.interchangecommerce.org/mailman/listinfo/interchange-users

On 28/06/20 3:41 am, Peter Ajamian wrote:
> On 26/06/20 7:04 am, Peter wrote:
>> On 26/06/20 4:18 am, Jon Jensen wrote:
>>> Peter,
>>>
>>> I just wanted to check in on your vlink.c and vlink.pl updates to
>>> respect the MINIVEND_SOCKET environment variable.
>>
>> Unfortuntately I haven't had time to test them yet I'v ejust been too
>> busy with other stuff.  I'll try and get that tested today or
>> tomorrow, though.
>
> ...tested, the vlink.c patch works well.  The vlink.pl patch has issues
> because vlink.pl is running with -T and $ENV{MINIVEND_SOCKET} is
> tainted.  If I untaint it then it works as well.  I'll update the patch
> and then push it out tomorrow.

Also ran across an issue with vlink.pl and had to rework the patch a
bit, but all tested and pushed out now.


Peter
_______________________________________________
interchange-users mailing list
interchange-users@interchangecommerce.org
https://www.interchangecommerce.org/mailman/listinfo/interchange-users