Mailing List Archive

On 1 Oct 2023, at 11:16, Erwin Hoffmann wrote:

> While qmail's and s/qmail's authentication is in principle not
> affected
> from the bug, I found a misbehaviour which may trigger a segfault of
> qmail-smtpd in case 'AUTH NTLM' is tried (which is never offered, of
> course):

This made me want to check authup.c, which was originally created six
years ago by merging the SMTP AUTH patch into qmail-popup.c and
refactoring until the POP3 and SMTP paths looked parallel enough for my
taste. (authup is part of acceptutils,
https://schmonz.com/software/acceptutils.)

A quick look at authup.c:smtp_auth()
(https://github.com/schmonz/qmail/blob/f057c5c23386fd6dd5293829d7ca49113a22e85c/authup.c#L502)
suggests to me that whatever problems my code has, they're different
ones. :-)

Thank you for investigating, sharing what you found, and reminding us to
do the same.

- Amitai

On Sun, 1 Oct 2023 at 20:53, Erwin Hoffmann <feh@fehcom.de> wrote:

> if (!authcmds[i].text) /* Minimal patch */
> if (!stralloc_copys(&authmethod,authcmds[i].text)) die_nomem();
>
> Shouldn't this be (because authcmds[i].txt is a NULL pointer when
unsupported auth method is used?

if (authcmds[i].txt)
if (!stralloc_copys(&authmethod, authcmds[i].txt)) die_nomem();

Hello Manvendra,

On Mon, Oct 02, 2023 at 02:02:58AM +0530, Manvendra Bhangui wrote:
> On Sun, 1 Oct 2023 at 20:53, Erwin Hoffmann <feh@fehcom.de> wrote:
>
> > if (!authcmds[i].text) /* Minimal patch */
> > if (!stralloc_copys(&authmethod,authcmds[i].text)) die_nomem();
> >
> > Shouldn't this be (because authcmds[i].txt is a NULL pointer when
> unsupported auth method is used?
>
> if (authcmds[i].txt)
> if (!stralloc_copys(&authmethod, authcmds[i].txt)) die_nomem();

Yes - but I have to warn about applying this patch unthinkingly. In recent
versions of s/qmail, it enables this code path a few lines down:

switch (authcmds[i].fun(arg)) {

authcmds[i].fun will be

int err_noauth()

and arg will be a string containing the unknown auth method supplied by the
SMTP client. This may be much worse than the null pointer dereference occuring
without the patch.

s/qmail 4.2.27 has just been released by Erwin and contains a proper patch.
Other variants of qmail with authentication patch will have to be looked over
by their respective authors.

Regards,

Michael Brunnbauer

--
++ Michael Brunnbauer
++ netEstate GmbH
++ Geisenhausener Stra?e 11a
++ 81379 M?nchen
++ Tel +49 89 32 19 77 80
++ Fax +49 89 32 19 77 89
++ E-Mail brunni@netestate.de
++ https://www.netestate.de/
++
++ Sitz: M?nchen, HRB Nr.142452 (Handelsregister B M?nchen)
++ USt-IdNr. DE221033342
++ Gesch?ftsf?hrer: Michael Brunnbauer
++ Prokurist: Dipl. Kfm. (Univ.) Markus Hendel

hi all,

I guess I'm a bit confused about the danger of passing arguments to functions
without prototype. Maybe all popular C-compilers actually handle that well?

Regards,

Michael Brunnbauer

On Sun, Oct 01, 2023 at 11:01:24PM +0200, Michael Brunnbauer wrote:
>
> Hello Manvendra,
>
> On Mon, Oct 02, 2023 at 02:02:58AM +0530, Manvendra Bhangui wrote:
> > On Sun, 1 Oct 2023 at 20:53, Erwin Hoffmann <feh@fehcom.de> wrote:
> >
> > > if (!authcmds[i].text) /* Minimal patch */
> > > if (!stralloc_copys(&authmethod,authcmds[i].text)) die_nomem();
> > >
> > > Shouldn't this be (because authcmds[i].txt is a NULL pointer when
> > unsupported auth method is used?
> >
> > if (authcmds[i].txt)
> > if (!stralloc_copys(&authmethod, authcmds[i].txt)) die_nomem();
>
> Yes - but I have to warn about applying this patch unthinkingly. In recent
> versions of s/qmail, it enables this code path a few lines down:
>
> switch (authcmds[i].fun(arg)) {
>
> authcmds[i].fun will be
>
> int err_noauth()
>
> and arg will be a string containing the unknown auth method supplied by the
> SMTP client. This may be much worse than the null pointer dereference occuring
> without the patch.
>
> s/qmail 4.2.27 has just been released by Erwin and contains a proper patch.
> Other variants of qmail with authentication patch will have to be looked over
> by their respective authors.
>
> Regards,
>
> Michael Brunnbauer
>
> --
> ++ Michael Brunnbauer
> ++ netEstate GmbH
> ++ Geisenhausener Stra?e 11a
> ++ 81379 M?nchen
> ++ Tel +49 89 32 19 77 80
> ++ Fax +49 89 32 19 77 89
> ++ E-Mail brunni@netestate.de
> ++ https://www.netestate.de/
> ++
> ++ Sitz: M?nchen, HRB Nr.142452 (Handelsregister B M?nchen)
> ++ USt-IdNr. DE221033342
> ++ Gesch?ftsf?hrer: Michael Brunnbauer
> ++ Prokurist: Dipl. Kfm. (Univ.) Markus Hendel



--
++ Michael Brunnbauer
++ netEstate GmbH
++ Geisenhausener Stra?e 11a
++ 81379 M?nchen
++ Tel +49 89 32 19 77 80
++ Fax +49 89 32 19 77 89
++ E-Mail brunni@netestate.de
++ https://www.netestate.de/
++
++ Sitz: M?nchen, HRB Nr.142452 (Handelsregister B M?nchen)
++ USt-IdNr. DE221033342
++ Gesch?ftsf?hrer: Michael Brunnbauer
++ Prokurist: Dipl. Kfm. (Univ.) Markus Hendel

>hi all,
>
>I guess I'm a bit confused about the danger of passing arguments to
functions without prototype. Maybe all popular C-compilers actually handle
that well?
>
>Regards,
>
>Michael Brunnbauer

Compilers have what is called an ABI, a convention for how arguments are
passed to called functions and how return values are returned to the caller.

https://en.wikipedia.org/wiki/Application_binary_interface

The conventions involve low-level machine stuff, like which CPU registers
are used for arguments of various types and sizes, how the stack frame is
set up and released, etc.

If the compiler doesn't see a prototype for a function that you would like
to call, or if it sees an incorrect prototype, it is possible when calling a
function that the compiler will set up CPU registers and the stack frame
(and interpret return values) in a way that is inconsistent with what the
called function expects and does. In that case, the results can vary from
harmless to subtly erroneous to catastrophic.

Harmless would be that the compiler guessed right, or that somehow the
misunderstanding was meaningless.

Subtly erroneous would be something like the called function is expecting 32
bits in two CPU registers, but the caller only sets up one of the CPU
registers (meaning that 16 bits are effectively random).

Catastrophic would be stack frame misunderstandings where an invalid pointer
or return address is used.

Are compilers better at it today? No. The only difference with modern
compilers is that they will generally issue a warning if they have not
encountered a prototype for a called function. That says nothing about
whether the prototype was correct.

General best practice is to provide "full prototype linkage", like this:

CALLED_FUNCTION.H

extern int my_called_function(unsigned long long x);

CALLED_FUNCTION.C

#include "called_function.h"

int my_called_function(unsigned long long x)
{
return((int) x *3); /* Or whatever */
}

CALLER.C

#include "called_function.h"

...
Whatever = my_called_function(42);
...


"Full prototype linkage" means:

a)In CALLED_FUNCTION.C, the compiler gets to compare the prototype against
the function definition, so it can generate unfriendly errors if the
prototype doesn't match the definition.
b)In CALLER.C, the compiler gets to see the prototype, so it can properly
compile the function call and possibly generate unfriendly errors.
c)In a transitive sense, the function call is compared against the function
definition (through the prototype). If A==B and B==C, then A==C.

But yeah, not having function prototypes is a Big Deal, particularly if it
generates subtle errors.

Hello David,

a friend tells me that usually, the caller is responsible for putting arguments
on the stack and removing them after the function returns. So a C-function
x() that is written as if it were x(void) but is called with an argument
should not pose a security problem in practice?

Regards,

Michael Brunnbauer

On Sun, Oct 01, 2023 at 08:29:14PM -0400, David T. Ashley wrote:
> >hi all,
> >
> >I guess I'm a bit confused about the danger of passing arguments to
> functions without prototype. Maybe all popular C-compilers actually handle
> that well?
> >
> >Regards,
> >
> >Michael Brunnbauer
>
> Compilers have what is called an ABI, a convention for how arguments are
> passed to called functions and how return values are returned to the caller.
>
> https://en.wikipedia.org/wiki/Application_binary_interface
>
> The conventions involve low-level machine stuff, like which CPU registers
> are used for arguments of various types and sizes, how the stack frame is
> set up and released, etc.
>
> If the compiler doesn't see a prototype for a function that you would like
> to call, or if it sees an incorrect prototype, it is possible when calling a
> function that the compiler will set up CPU registers and the stack frame
> (and interpret return values) in a way that is inconsistent with what the
> called function expects and does. In that case, the results can vary from
> harmless to subtly erroneous to catastrophic.
>
> Harmless would be that the compiler guessed right, or that somehow the
> misunderstanding was meaningless.
>
> Subtly erroneous would be something like the called function is expecting 32
> bits in two CPU registers, but the caller only sets up one of the CPU
> registers (meaning that 16 bits are effectively random).
>
> Catastrophic would be stack frame misunderstandings where an invalid pointer
> or return address is used.
>
> Are compilers better at it today? No. The only difference with modern
> compilers is that they will generally issue a warning if they have not
> encountered a prototype for a called function. That says nothing about
> whether the prototype was correct.
>
> General best practice is to provide "full prototype linkage", like this:
>
> CALLED_FUNCTION.H
>
> extern int my_called_function(unsigned long long x);
>
> CALLED_FUNCTION.C
>
> #include "called_function.h"
>
> int my_called_function(unsigned long long x)
> {
> return((int) x *3); /* Or whatever */
> }
>
> CALLER.C
>
> #include "called_function.h"
>
> ...
> Whatever = my_called_function(42);
> ...
>
>
> "Full prototype linkage" means:
>
> a)In CALLED_FUNCTION.C, the compiler gets to compare the prototype against
> the function definition, so it can generate unfriendly errors if the
> prototype doesn't match the definition.
> b)In CALLER.C, the compiler gets to see the prototype, so it can properly
> compile the function call and possibly generate unfriendly errors.
> c)In a transitive sense, the function call is compared against the function
> definition (through the prototype). If A==B and B==C, then A==C.
>
> But yeah, not having function prototypes is a Big Deal, particularly if it
> generates subtle errors.
>

--
++ Michael Brunnbauer
++ netEstate GmbH
++ Geisenhausener Stra?e 11a
++ 81379 M?nchen
++ Tel +49 89 32 19 77 80
++ Fax +49 89 32 19 77 89
++ E-Mail brunni@netestate.de
++ https://www.netestate.de/
++
++ Sitz: M?nchen, HRB Nr.142452 (Handelsregister B M?nchen)
++ USt-IdNr. DE221033342
++ Gesch?ftsf?hrer: Michael Brunnbauer
++ Prokurist: Dipl. Kfm. (Univ.) Markus Hendel

I was speaking in general terms about what can go wrong if there is not full
function prototype linkage.

As far as the specific case of a function expecting no arguments but
receiving one ... I'm not sure what the standards say about this. I would
suspect that there are no guarantees, although it might fall under the topic
of "variadic arguments".

The reason I suspect there are "no guarantees" is that the most convenient
place to store return addresses on the stack, the most efficient way to
access parameters on the stack, and the most efficient way to clean up the
stack get into machine architecture, and machines may be different.

I suspect that in the typical case there would be no issue (simply because
modern machines have so many registers and a typical ABI would put a single
parameter in a CPU register), but I'm not sure the C standard provides any
guarantees that something could not go seriously wrong.

>Hello David,
>
>a friend tells me that usually, the caller is responsible for putting
arguments on the stack and removing them after the function returns. So a
C-function
>x() that is written as if it were x(void) but is called with an argument
should not pose a security problem in practice?
>
>Regards,
>
>Michael Brunnbauer
>
>On Sun, Oct 01, 2023 at 08:29:14PM -0400, David T. Ashley wrote:
> >hi all,
> >
> >I guess I'm a bit confused about the danger of passing arguments to
> functions without prototype. Maybe all popular C-compilers actually
> handle that well?
> >
> >Regards,
> >
> >Michael Brunnbauer
>
> Compilers have what is called an ABI, a convention for how arguments
> are passed to called functions and how return values are returned to the
caller.
>
> https://en.wikipedia.org/wiki/Application_binary_interface
>
> The conventions involve low-level machine stuff, like which CPU
> registers are used for arguments of various types and sizes, how the
> stack frame is set up and released, etc.
>
> If the compiler doesn't see a prototype for a function that you would
> like to call, or if it sees an incorrect prototype, it is possible
> when calling a function that the compiler will set up CPU registers
> and the stack frame (and interpret return values) in a way that is
> inconsistent with what the called function expects and does. In that
> case, the results can vary from harmless to subtly erroneous to
catastrophic.
>
> Harmless would be that the compiler guessed right, or that somehow the
> misunderstanding was meaningless.
>
> Subtly erroneous would be something like the called function is
> expecting 32 bits in two CPU registers, but the caller only sets up
> one of the CPU registers (meaning that 16 bits are effectively random).
>
> Catastrophic would be stack frame misunderstandings where an invalid
> pointer or return address is used.
>
> Are compilers better at it today? No. The only difference with
> modern compilers is that they will generally issue a warning if they
> have not encountered a prototype for a called function. That says
> nothing about whether the prototype was correct.
>
> General best practice is to provide "full prototype linkage", like this:
>
> CALLED_FUNCTION.H
>
> extern int my_called_function(unsigned long long x);
>
> CALLED_FUNCTION.C
>
> #include "called_function.h"
>
> int my_called_function(unsigned long long x) {
> return((int) x *3); /* Or whatever */ }
>
> CALLER.C
>
> #include "called_function.h"
>
> ...
> Whatever = my_called_function(42);
> ...
>
>
> "Full prototype linkage" means:
>
> a)In CALLED_FUNCTION.C, the compiler gets to compare the prototype
> against the function definition, so it can generate unfriendly errors
> if the prototype doesn't match the definition.
> b)In CALLER.C, the compiler gets to see the prototype, so it can
> properly compile the function call and possibly generate unfriendly
errors.
> c)In a transitive sense, the function call is compared against the
> function definition (through the prototype). If A==B and B==C, then A==C.
>
> But yeah, not having function prototypes is a Big Deal, particularly
> if it generates subtle errors.