Probable off by one in src/usr.bin/rdist/docmd.c

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Probable off by one in src/usr.bin/rdist/docmd.c

Aham Brahmasmi
Namaste misc,

Question:
In the makeconn function in src/usr.bin/rdist/docmd.c, should the 5 in
the following line be replaced by 4?
...
static int
makeconn(char *rhost)
{
...
(void) snprintf(buf, sizeof(buf), "%.*s -S",
                        (int)(sizeof(buf)-5), path_rdistd);
...

Explanation:
I have a limited ability to read code, so I may be wrong here.

If I am not wrong, strings are terminated with '\0' which I think is a
single byte. So, in the above case, the sizeof(" -S" + '\0')=4. But the
code has 5.

I am not sure of my "'\0' is a single byte" part, and hence my query.

Dhanyavaad,
ab
[1] - https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/rdist/docmd.c?rev=1.34
---------|---------|---------|---------|---------|---------|---------|--

Reply | Threaded
Open this post in threaded view
|

Re: Probable off by one in src/usr.bin/rdist/docmd.c

Otto Moerbeek
On Wed, Jan 01, 2020 at 04:02:24PM +0100, Aham Brahmasmi wrote:

> Namaste misc,
>
> Question:
> In the makeconn function in src/usr.bin/rdist/docmd.c, should the 5 in
> the following line be replaced by 4?
> ...
> static int
> makeconn(char *rhost)
> {
> ...
> (void) snprintf(buf, sizeof(buf), "%.*s -S",
> (int)(sizeof(buf)-5), path_rdistd);
> ...
>
> Explanation:
> I have a limited ability to read code, so I may be wrong here.
>
> If I am not wrong, strings are terminated with '\0' which I think is a
> single byte. So, in the above case, the sizeof(" -S" + '\0')=4. But the
> code has 5.
>
> I am not sure of my "'\0' is a single byte" part, and hence my query.

By definition, '\0' is a single byte. sizeof(String literal) included
the terminating '\0'. So sizeof("foo") is 4.

The sizeof(buf)-5 fills in the precision of the %....s. That means
that path_rdistd wil be limited to that number of chars. The " -S"
part indeed takes 3 chars, so there is sizeof(buf) - 3 left for
path_rdistd, excluding the terminating '\0'. So -4 is indeed right.

Butt does it matter? I'd say no, only if path_rdistd is close to
BUFSIZ in length tunrcation will happen 1 char earlier than possible.
I would argue that specifying the precision here is rather confusing,
and it would be better to use the standard idiom equivalent to the
example in the snprintf man page.

        -Otto

Reply | Threaded
Open this post in threaded view
|

Re: Probable off by one in src/usr.bin/rdist/docmd.c

Aham Brahmasmi
Hallo Otto,

Dank je Otto for your helpful reply.

> Sent: Wednesday, January 01, 2020 at 3:36 PM
> From: "Otto Moerbeek" <[hidden email]>
> To: "Aham Brahmasmi" <[hidden email]>
> Cc: [hidden email]
> Subject: Re: Probable off by one in src/usr.bin/rdist/docmd.c
>
> On Wed, Jan 01, 2020 at 04:02:24PM +0100, Aham Brahmasmi wrote:
>
> > Namaste misc,
> >
> > Question:
> > In the makeconn function in src/usr.bin/rdist/docmd.c, should the 5 in
> > the following line be replaced by 4?
> > ...
> > static int
> > makeconn(char *rhost)
> > {
> > ...
> > (void) snprintf(buf, sizeof(buf), "%.*s -S",
> > (int)(sizeof(buf)-5), path_rdistd);
> > ...
> >
> > Explanation:
> > I have a limited ability to read code, so I may be wrong here.
> >
> > If I am not wrong, strings are terminated with '\0' which I think is a
> > single byte. So, in the above case, the sizeof(" -S" + '\0')=4. But the
> > code has 5.
> >
> > I am not sure of my "'\0' is a single byte" part, and hence my query.
>
> By definition, '\0' is a single byte. sizeof(String literal) included
> the terminating '\0'. So sizeof("foo") is 4.
>
> The sizeof(buf)-5 fills in the precision of the %....s. That means
> that path_rdistd wil be limited to that number of chars. The " -S"
> part indeed takes 3 chars, so there is sizeof(buf) - 3 left for
> path_rdistd, excluding the terminating '\0'. So -4 is indeed right.

Understood.

> Butt does it matter? I'd say no, only if path_rdistd is close to
> BUFSIZ in length tunrcation will happen 1 char earlier than possible.
> I would argue that specifying the precision here is rather confusing,
> and it would be better to use the standard idiom equivalent to the
> example in the snprintf man page.

From the snprintf man page (https://man.openbsd.org/snprintf):

...
int
snprintf(char *str, size_t size, const char *format, ...);
...

So, if I understand the standard idiom in the snprintf man page
correctly, the modified line would be:

(void) snprintf(buf, sizeof(buf), "%s -S", path_rdistd);

Am I correct in my understanding?

> -Otto

Dhanyavaad,
ab
---------|---------|---------|---------|---------|---------|---------|--

Reply | Threaded
Open this post in threaded view
|

Re: Probable off by one in src/usr.bin/rdist/docmd.c

Otto Moerbeek
On Thu, Jan 02, 2020 at 03:39:53PM +0100, Aham Brahmasmi wrote:

> Hallo Otto,
>
> Dank je Otto for your helpful reply.
>
> > Sent: Wednesday, January 01, 2020 at 3:36 PM
> > From: "Otto Moerbeek" <[hidden email]>
> > To: "Aham Brahmasmi" <[hidden email]>
> > Cc: [hidden email]
> > Subject: Re: Probable off by one in src/usr.bin/rdist/docmd.c
> >
> > On Wed, Jan 01, 2020 at 04:02:24PM +0100, Aham Brahmasmi wrote:
> >
> > > Namaste misc,
> > >
> > > Question:
> > > In the makeconn function in src/usr.bin/rdist/docmd.c, should the 5 in
> > > the following line be replaced by 4?
> > > ...
> > > static int
> > > makeconn(char *rhost)
> > > {
> > > ...
> > > (void) snprintf(buf, sizeof(buf), "%.*s -S",
> > > (int)(sizeof(buf)-5), path_rdistd);
> > > ...
> > >
> > > Explanation:
> > > I have a limited ability to read code, so I may be wrong here.
> > >
> > > If I am not wrong, strings are terminated with '\0' which I think is a
> > > single byte. So, in the above case, the sizeof(" -S" + '\0')=4. But the
> > > code has 5.
> > >
> > > I am not sure of my "'\0' is a single byte" part, and hence my query.
> >
> > By definition, '\0' is a single byte. sizeof(String literal) included
> > the terminating '\0'. So sizeof("foo") is 4.
> >
> > The sizeof(buf)-5 fills in the precision of the %....s. That means
> > that path_rdistd wil be limited to that number of chars. The " -S"
> > part indeed takes 3 chars, so there is sizeof(buf) - 3 left for
> > path_rdistd, excluding the terminating '\0'. So -4 is indeed right.
>
> Understood.
>
> > Butt does it matter? I'd say no, only if path_rdistd is close to
> > BUFSIZ in length tunrcation will happen 1 char earlier than possible.
> > I would argue that specifying the precision here is rather confusing,
> > and it would be better to use the standard idiom equivalent to the
> > example in the snprintf man page.
>
> From the snprintf man page (https://man.openbsd.org/snprintf):
>
> ...
> int
> snprintf(char *str, size_t size, const char *format, ...);
> ...
>
> So, if I understand the standard idiom in the snprintf man page
> correctly, the modified line would be:
>
> (void) snprintf(buf, sizeof(buf), "%s -S", path_rdistd);
>
> Am I correct in my understanding?
>
> > -Otto
>
> Dhanyavaad,
> ab

No,

you want to check for truncation. See the CAVEATS section.

        -Otto

Reply | Threaded
Open this post in threaded view
|

Re: Probable off by one in src/usr.bin/rdist/docmd.c

Aham Brahmasmi
> Sent: Thursday, January 02, 2020 at 4:26 PM
> From: "Otto Moerbeek" <[hidden email]>
> To: "Aham Brahmasmi" <[hidden email]>
> Cc: [hidden email]
> Subject: Re: Probable off by one in src/usr.bin/rdist/docmd.c
>
> On Thu, Jan 02, 2020 at 03:39:53PM +0100, Aham Brahmasmi wrote:
>
> > Hallo Otto,
> >
> > Dank je Otto for your helpful reply.
> >
> > > Sent: Wednesday, January 01, 2020 at 3:36 PM
> > > From: "Otto Moerbeek" <[hidden email]>
> > > To: "Aham Brahmasmi" <[hidden email]>
> > > Cc: [hidden email]
> > > Subject: Re: Probable off by one in src/usr.bin/rdist/docmd.c
> > >
> > > On Wed, Jan 01, 2020 at 04:02:24PM +0100, Aham Brahmasmi wrote:
> > >
> > > > Namaste misc,
> > > >
> > > > Question:
> > > > In the makeconn function in src/usr.bin/rdist/docmd.c, should the 5 in
> > > > the following line be replaced by 4?
> > > > ...
> > > > static int
> > > > makeconn(char *rhost)
> > > > {
> > > > ...
> > > > (void) snprintf(buf, sizeof(buf), "%.*s -S",
> > > > (int)(sizeof(buf)-5), path_rdistd);
> > > > ...
> > > >
> > > > Explanation:
> > > > I have a limited ability to read code, so I may be wrong here.
> > > >
> > > > If I am not wrong, strings are terminated with '\0' which I think is a
> > > > single byte. So, in the above case, the sizeof(" -S" + '\0')=4. But the
> > > > code has 5.
> > > >
> > > > I am not sure of my "'\0' is a single byte" part, and hence my query.
> > >
> > > By definition, '\0' is a single byte. sizeof(String literal) included
> > > the terminating '\0'. So sizeof("foo") is 4.
> > >
> > > The sizeof(buf)-5 fills in the precision of the %....s. That means
> > > that path_rdistd wil be limited to that number of chars. The " -S"
> > > part indeed takes 3 chars, so there is sizeof(buf) - 3 left for
> > > path_rdistd, excluding the terminating '\0'. So -4 is indeed right.
> >
> > Understood.
> >
> > > Butt does it matter? I'd say no, only if path_rdistd is close to
> > > BUFSIZ in length tunrcation will happen 1 char earlier than possible.
> > > I would argue that specifying the precision here is rather confusing,
> > > and it would be better to use the standard idiom equivalent to the
> > > example in the snprintf man page.
> >
> > From the snprintf man page (https://man.openbsd.org/snprintf):
> >
> > ...
> > int
> > snprintf(char *str, size_t size, const char *format, ...);
> > ...
> >
> > So, if I understand the standard idiom in the snprintf man page
> > correctly, the modified line would be:
> >
> > (void) snprintf(buf, sizeof(buf), "%s -S", path_rdistd);
> >
> > Am I correct in my understanding?
> >
> > > -Otto
> >
> > Dhanyavaad,
> > ab
>
> No,
>
> you want to check for truncation. See the CAVEATS section.
>
> -Otto

Ah ok, CAVEATS. My bad. The return value of the function should be
checked for possible error conditions. Dank je Otto.

So, if I understand correctly, checking for truncation implies ensuring
that the input string was not truncated to fit in the buffer.

So, the modified line would be:

int ret = snprintf(buf, sizeof(buf), "%s -S", path_rdistd);

if (ret < 0) {
        // Some error has occurred.
        // goto error;
}
if (ret >= sizeof(buf)) {
        // The input string is longer than the size of the buffer and
        // hence has been truncated to fit into the buffer.
        // goto toolong;
}

Is my understanding of the check for truncation correct?

Dhanyavaad,
ab
---------|---------|---------|---------|---------|---------|---------|--

Reply | Threaded
Open this post in threaded view
|

Re: Probable off by one in src/usr.bin/rdist/docmd.c

Otto Moerbeek
On Thu, Jan 02, 2020 at 07:45:25PM +0100, Aham Brahmasmi wrote:

> > Sent: Thursday, January 02, 2020 at 4:26 PM
> > From: "Otto Moerbeek" <[hidden email]>
> > To: "Aham Brahmasmi" <[hidden email]>
> > Cc: [hidden email]
> > Subject: Re: Probable off by one in src/usr.bin/rdist/docmd.c
> >
> > On Thu, Jan 02, 2020 at 03:39:53PM +0100, Aham Brahmasmi wrote:
> >
> > > Hallo Otto,
> > >
> > > Dank je Otto for your helpful reply.
> > >
> > > > Sent: Wednesday, January 01, 2020 at 3:36 PM
> > > > From: "Otto Moerbeek" <[hidden email]>
> > > > To: "Aham Brahmasmi" <[hidden email]>
> > > > Cc: [hidden email]
> > > > Subject: Re: Probable off by one in src/usr.bin/rdist/docmd.c
> > > >
> > > > On Wed, Jan 01, 2020 at 04:02:24PM +0100, Aham Brahmasmi wrote:
> > > >
> > > > > Namaste misc,
> > > > >
> > > > > Question:
> > > > > In the makeconn function in src/usr.bin/rdist/docmd.c, should the 5 in
> > > > > the following line be replaced by 4?
> > > > > ...
> > > > > static int
> > > > > makeconn(char *rhost)
> > > > > {
> > > > > ...
> > > > > (void) snprintf(buf, sizeof(buf), "%.*s -S",
> > > > > (int)(sizeof(buf)-5), path_rdistd);
> > > > > ...
> > > > >
> > > > > Explanation:
> > > > > I have a limited ability to read code, so I may be wrong here.
> > > > >
> > > > > If I am not wrong, strings are terminated with '\0' which I think is a
> > > > > single byte. So, in the above case, the sizeof(" -S" + '\0')=4. But the
> > > > > code has 5.
> > > > >
> > > > > I am not sure of my "'\0' is a single byte" part, and hence my query.
> > > >
> > > > By definition, '\0' is a single byte. sizeof(String literal) included
> > > > the terminating '\0'. So sizeof("foo") is 4.
> > > >
> > > > The sizeof(buf)-5 fills in the precision of the %....s. That means
> > > > that path_rdistd wil be limited to that number of chars. The " -S"
> > > > part indeed takes 3 chars, so there is sizeof(buf) - 3 left for
> > > > path_rdistd, excluding the terminating '\0'. So -4 is indeed right.
> > >
> > > Understood.
> > >
> > > > Butt does it matter? I'd say no, only if path_rdistd is close to
> > > > BUFSIZ in length tunrcation will happen 1 char earlier than possible.
> > > > I would argue that specifying the precision here is rather confusing,
> > > > and it would be better to use the standard idiom equivalent to the
> > > > example in the snprintf man page.
> > >
> > > From the snprintf man page (https://man.openbsd.org/snprintf):
> > >
> > > ...
> > > int
> > > snprintf(char *str, size_t size, const char *format, ...);
> > > ...
> > >
> > > So, if I understand the standard idiom in the snprintf man page
> > > correctly, the modified line would be:
> > >
> > > (void) snprintf(buf, sizeof(buf), "%s -S", path_rdistd);
> > >
> > > Am I correct in my understanding?
> > >
> > > > -Otto
> > >
> > > Dhanyavaad,
> > > ab
> >
> > No,
> >
> > you want to check for truncation. See the CAVEATS section.
> >
> > -Otto
>
> Ah ok, CAVEATS. My bad. The return value of the function should be
> checked for possible error conditions. Dank je Otto.
>
> So, if I understand correctly, checking for truncation implies ensuring
> that the input string was not truncated to fit in the buffer.
>
> So, the modified line would be:
>
> int ret = snprintf(buf, sizeof(buf), "%s -S", path_rdistd);
>
> if (ret < 0) {
> // Some error has occurred.
> // goto error;
> }
> if (ret >= sizeof(buf)) {
> // The input string is longer than the size of the buffer and
> // hence has been truncated to fit into the buffer.
> // goto toolong;
> }
>
> Is my understanding of the check for truncation correct?
>
> Dhanyavaad,
> ab

Look, I've done already enough hand-holding. There are pelnty pf
resources to educate yourself. At some point you need to stop asking
question and study.

        -Otto

Reply | Threaded
Open this post in threaded view
|

Re: Probable off by one in src/usr.bin/rdist/docmd.c

Aham Brahmasmi
> Sent: Thursday, January 02, 2020 at 8:21 PM
> From: "Otto Moerbeek" <[hidden email]>
> To: "Aham Brahmasmi" <[hidden email]>
> Cc: [hidden email]
> Subject: Re: Probable off by one in src/usr.bin/rdist/docmd.c
>
> On Thu, Jan 02, 2020 at 07:45:25PM +0100, Aham Brahmasmi wrote:
>
> > > Sent: Thursday, January 02, 2020 at 4:26 PM
> > > From: "Otto Moerbeek" <[hidden email]>
> > > To: "Aham Brahmasmi" <[hidden email]>
> > > Cc: [hidden email]
> > > Subject: Re: Probable off by one in src/usr.bin/rdist/docmd.c
> > >
> > > On Thu, Jan 02, 2020 at 03:39:53PM +0100, Aham Brahmasmi wrote:
> > >
> > > > Hallo Otto,
> > > >
> > > > Dank je Otto for your helpful reply.
> > > >
> > > > > Sent: Wednesday, January 01, 2020 at 3:36 PM
> > > > > From: "Otto Moerbeek" <[hidden email]>
> > > > > To: "Aham Brahmasmi" <[hidden email]>
> > > > > Cc: [hidden email]
> > > > > Subject: Re: Probable off by one in src/usr.bin/rdist/docmd.c
> > > > >
> > > > > On Wed, Jan 01, 2020 at 04:02:24PM +0100, Aham Brahmasmi wrote:
> > > > >
> > > > > > Namaste misc,
> > > > > >
> > > > > > Question:
> > > > > > In the makeconn function in src/usr.bin/rdist/docmd.c, should the 5 in
> > > > > > the following line be replaced by 4?
> > > > > > ...
> > > > > > static int
> > > > > > makeconn(char *rhost)
> > > > > > {
> > > > > > ...
> > > > > > (void) snprintf(buf, sizeof(buf), "%.*s -S",
> > > > > > (int)(sizeof(buf)-5), path_rdistd);
> > > > > > ...
> > > > > >
> > > > > > Explanation:
> > > > > > I have a limited ability to read code, so I may be wrong here.
> > > > > >
> > > > > > If I am not wrong, strings are terminated with '\0' which I think is a
> > > > > > single byte. So, in the above case, the sizeof(" -S" + '\0')=4. But the
> > > > > > code has 5.
> > > > > >
> > > > > > I am not sure of my "'\0' is a single byte" part, and hence my query.
> > > > >
> > > > > By definition, '\0' is a single byte. sizeof(String literal) included
> > > > > the terminating '\0'. So sizeof("foo") is 4.
> > > > >
> > > > > The sizeof(buf)-5 fills in the precision of the %....s. That means
> > > > > that path_rdistd wil be limited to that number of chars. The " -S"
> > > > > part indeed takes 3 chars, so there is sizeof(buf) - 3 left for
> > > > > path_rdistd, excluding the terminating '\0'. So -4 is indeed right.
> > > >
> > > > Understood.
> > > >
> > > > > Butt does it matter? I'd say no, only if path_rdistd is close to
> > > > > BUFSIZ in length tunrcation will happen 1 char earlier than possible.
> > > > > I would argue that specifying the precision here is rather confusing,
> > > > > and it would be better to use the standard idiom equivalent to the
> > > > > example in the snprintf man page.
> > > >
> > > > From the snprintf man page (https://man.openbsd.org/snprintf):
> > > >
> > > > ...
> > > > int
> > > > snprintf(char *str, size_t size, const char *format, ...);
> > > > ...
> > > >
> > > > So, if I understand the standard idiom in the snprintf man page
> > > > correctly, the modified line would be:
> > > >
> > > > (void) snprintf(buf, sizeof(buf), "%s -S", path_rdistd);
> > > >
> > > > Am I correct in my understanding?
> > > >
> > > > > -Otto
> > > >
> > > > Dhanyavaad,
> > > > ab
> > >
> > > No,
> > >
> > > you want to check for truncation. See the CAVEATS section.
> > >
> > > -Otto
> >
> > Ah ok, CAVEATS. My bad. The return value of the function should be
> > checked for possible error conditions. Dank je Otto.
> >
> > So, if I understand correctly, checking for truncation implies ensuring
> > that the input string was not truncated to fit in the buffer.
> >
> > So, the modified line would be:
> >
> > int ret = snprintf(buf, sizeof(buf), "%s -S", path_rdistd);
> >
> > if (ret < 0) {
> > // Some error has occurred.
> > // goto error;
> > }
> > if (ret >= sizeof(buf)) {
> > // The input string is longer than the size of the buffer and
> > // hence has been truncated to fit into the buffer.
> > // goto toolong;
> > }
> >
> > Is my understanding of the check for truncation correct?
> >
> > Dhanyavaad,
> > ab
>
> Look, I've done already enough hand-holding. There are pelnty pf
> resources to educate yourself. At some point you need to stop asking
> question and study.
>
> -Otto

Understood. Dank je Otto for your helpful direction - I learnt some new
things, although as I now understand, I need to keep learning.

Actually, I landed up on that snprintf line in rdist because of a blog
post by the good volks at OpenBSD Amsterdam.

On https://high5.nl/gist/rdist.html, the obsdams volks have written an
informative blog post outlining their usage of rdist. This post was in
turn inspired by another helpful post on rdist by Johan Huldtgren at
http://johan.huldtgren.com/posts/2019/rdist. In Johan's post, he gives
an interesting reader's exercise, which was solved by the obsdams volks
by wrapping rdist in a shell invocation with doas. I thought, may be,
this could be improved upon, since the wrapping seemed brittle to me.

For this, I read the rdist manpage (https://man.openbsd.org/rdist) and
encountered the "-p rdistd-path" flag
...
-p rdistd-path
    Set the path where the rdistd server is searched for on the target host.
...

To better understand what that flag does, I read the rdist code.

The -p flag, if passed, is captured in the path_rdistd variable. The
default for path_rdistd is "rdistd". And the snprintf line is where the
path_rdistd is concatenated with " -S", the default invocation flag of
rdistd.

So, if I am not wrong, the obsdams volks might benefit from adding
-p "doas rdistd" to their rdist invocation on the source host and
restoring rdistd-orig to rdistd on the destination host. If this works,
it might save them the potential headache of maintaining rdistd as a
shell script, since the shell script might get overwritten with the
rdistd binary after every base update.

Hope this helps. Dank je for your time.

Dhanyavaad,
ab
---------|---------|---------|---------|---------|---------|---------|--