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 :-/
Willy
