On Wed, Oct 04, 2017 at 05:07:07AM +0200, Willy Tarreau wrote:
> On Wed, Oct 04, 2017 at 04:40:53AM +0200, Willy Tarreau wrote:
> > On Tue, Oct 03, 2017 at 06:57:45PM +0200, Marcus Ulbrich wrote:
> > > Hey Jarno,
> > >
> > > This seems to work stable!
> > > The idea for this acl was to prevent attackers testing for example MySQL
> > > injection by sleep command. ,,sleep" is in none of our URLs!
> > > Do you have an idea about an acl about this not crashing haproxy?
> >
> > I wouldn't be surprized if there was an issue for example with a %00 causing
> > a length mismatch between two parts. At least now we have an idea where to
> > look for the bug. It shouldn't take very long anymore to spot the problem.
>
> I'm seeing an issue in the url_dec sample converter :
>
> static int sample_conv_url_dec(const struct arg *args, struct sample
> *smp, void
> *private)
> {
> /* If the constant flag is set or if not size is avalaible at
> * the end of the buffer, copy the string in other buffer
> * before decoding.
> */
> if (smp->flags & SMP_F_CONST || smp->data.u.str.size <=
> smp->data.u.str.
> len) {
> struct chunk *str = get_trash_chunk();
> memcpy(str->str, smp->data.u.str.str,
> smp->data.u.str.len);
>
> Here it's missing a check to ensure that str.len <= str.size. Normally there
> is no known case where this could happen, but the preferred smp_dup() performs
> the check and truncates the input if needed. I don't know if there's a reason
> for this to ever happen. Hmmm in fact there's one theorical case where it
> could
> happen, it's if the sample contains a full buffer (str.len == str.size), and
> then we can overflow the block by one byte when writing the trailing zero. But
> it's not possible in HTTP since the request is at least a few bytes long, and
> there's the reserve.
>
> smp->data.u.str.str = str->str;
> smp->data.u.str.size = str->size;
> smp->flags &= ~SMP_F_CONST;
> }
>
> So if you're interested, you can replace the block above by this :
> if (smp->flags & SMP_F_CONST || smp->data.u.str.size <=
> smp->data.u.str.
> len)
> smp_dup(smp);
>
> If this stops crashing we'll have to try to figure in which situation the
> case above may happen :-/
By the way if you manage to print the contents of "smp" in
sample_conv_str2lower() it could help, at least to find smp->str.{len,size}.
Willy
