Bug #99748 for DBD-SQLite: The gnerated virtual table perl code does not seem to handle NULL

Thu Oct 23 04:22:29 2014 ribasushi [...] leporine.io - Ticket created

CC:	laurent.dami [...] free.fr
Subject:	The gnerated virtual table perl code does not seem to handle NULL

As in the topic - there seems to be no provision/testing for NULL/undef data. Also the escape of the string-to-be-evaled seems rather risky - I would strongly recommend using quotemeta instead: https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped-0.10012/lib/Class/Accessor/Grouped.pm#L74 This may or may not be a security problem, I didn't investigate possibilities.

Fri Oct 24 23:45:09 2014 DAMI [...] cpan.org - Taken

Sat Oct 25 00:11:33 2014 laurent.dami [...] free.fr - Correspondence added

Subject:	Re: [rt.cpan.org #99748] The gnerated virtual table perl code does not seem to handle NULL
Date:	Sat, 25 Oct 2014 06:11:17 +0200
To:	bug-DBD-SQLite [...] rt.cpan.org
From:	Laurent Dami <laurent.dami [...] free.fr>

Le 23.10.2014 10:22, Peter Rabbitson via RT a écrit : Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=99748 > > > As in the topic - there seems to be no provision/testing for NULL/undef data.

Thanks Riba, your are right, I merely forgot that NULLs belong to SQL :-( Show quoted text

> Also the escape of the string-to-be-evaled seems rather risky - I would strongly recommend using quotemeta instead: https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped-0.10012/lib/Class/Accessor/Grouped.pm#L74 > > This may or may not be a security problem, I didn't investigate possibilities.

Well, in most cases values get interpolated within a q{}, which is why only backslash and closing brace were escaped. But with the MATCH operator, the interpolation goes within a qr{}, and there your are right, this could be a security hole. So I'll follow your advice with quotemeta(). Thanks for this detailed investigation !

Sat Oct 25 00:11:33 2014 The RT System itself - Status changed from 'new' to 'open'

Sat Oct 25 15:23:38 2014 laurent.dami [...] free.fr - Correspondence added

Subject:	Re: [rt.cpan.org #99748] The gnerated virtual table perl code does not seem to handle NULL
Date:	Sat, 25 Oct 2014 21:23:26 +0200
To:	bug-DBD-SQLite [...] rt.cpan.org
From:	Laurent Dami <laurent.dami [...] free.fr>

Conditions for testing undef (NULL) have been added (https://github.com/DBD-SQLite/DBD-SQLite/commit/c6d9c27e22acb4235e2ba3e3dcf42631534cf054). As for security breaches, I added a few tests for eval-in-regex operators, or for interpolations with embedded code -- everything seems to be safe. I cannot use quotemeta() as suggested, because we do want special characters like \n or [aeiou] to have their usual meaning in regexes. For the same reason, we cannot use the single quote as delimiter, which would prevent any interpolation. I'm quite confident that no code insertion can be performed; but still there is the annoying fact that the regex could possibly contain interpolations of variables like $self, $0 or @main::ARGV, and unfortunately I don't see how to prevent that. Any suggestion ? Anyway, such interpolations within regexes could cause exceptions; but I can't think of any security exploit based on this. So, since it's very unlikely to happen, and not very dangerous if it happens, there is no big risk.

Sat Oct 25 15:27:51 2014 DAMI [...] cpan.org - Status changed from 'open' to 'patched'

Sat Oct 25 22:36:50 2014 ishigaki [...] cpan.org - Correspondence added

RT-Send-CC:

laurent.dami [...] free.fr

I haven't looked into details yet but t/virtual/rt_99748.t is broken under... 1) Perl 5.8.8 and below (due to segfaults) 2) Perl 5.18.0 and above (due to experimental warning) On Sun Oct 26 04:23:38 2014, laurent.dami@free.fr wrote: Show quoted text

> Conditions for testing undef (NULL) have been added > (https://github.com/DBD-SQLite/DBD- > SQLite/commit/c6d9c27e22acb4235e2ba3e3dcf42631534cf054). > > As for security breaches, I added a few tests for eval-in-regex > operators, or for interpolations with embedded code -- everything > seems > to be safe. I cannot use quotemeta() as suggested, because we do want > special characters like \n or [aeiou] to have their usual meaning in > regexes. For the same reason, we cannot use the single quote as > delimiter, which would prevent any interpolation. > > I'm quite confident that no code insertion can be performed; but still > there is the annoying fact that the regex could possibly contain > interpolations of variables like $self, $0 or @main::ARGV, and > unfortunately I don't see how to prevent that. Any suggestion ? > > Anyway, such interpolations within regexes could cause exceptions; but > I > can't think of any security exploit based on this. So, since it's very > unlikely to happen, and not very dangerous if it happens, there is no > big risk.

Sun Oct 26 05:56:58 2014 MAUKE [...] cpan.org - Correspondence added

On Sat Oct 25 15:23:38 2014, laurent.dami@free.fr wrote: Show quoted text

> Conditions for testing undef (NULL) have been added > (https://github.com/DBD-SQLite/DBD- > SQLite/commit/c6d9c27e22acb4235e2ba3e3dcf42631534cf054). > > As for security breaches, I added a few tests for eval-in-regex > operators, or for interpolations with embedded code -- everything > seems > to be safe.

I think that's only because the code doesn't handle '{' in strings. Even something simple like "WHERE foo = '{'" would fail. I cannot use quotemeta() as suggested, because we do want Show quoted text

> special characters like \n or [aeiou] to have their usual meaning in > regexes. For the same reason, we cannot use the single quote as > delimiter, which would prevent any interpolation. > > I'm quite confident that no code insertion can be performed; but still > there is the annoying fact that the regex could possibly contain > interpolations of variables like $self, $0 or @main::ARGV, and > unfortunately I don't see how to prevent that. Any suggestion ?

Code injection and interpolation are the same thing. Consider "WHERE foo MATCH '$x[$y + system "rm -rf ~"]'". Show quoted text

> > Anyway, such interpolations within regexes could cause exceptions; but > I > can't think of any security exploit based on this. So, since it's very > unlikely to happen, and not very dangerous if it happens, there is no > big risk.

Mon Oct 27 23:53:46 2014 ishigaki [...] cpan.org - Correspondence added

RT-Send-CC:

laurent.dami [...] free.fr

On Sun Oct 26 18:56:58 2014, MAUKE wrote: Show quoted text

> On Sat Oct 25 15:23:38 2014, laurent.dami@free.fr wrote:

> > Conditions for testing undef (NULL) have been added > > (https://github.com/DBD-SQLite/DBD- > > SQLite/commit/c6d9c27e22acb4235e2ba3e3dcf42631534cf054). > > > > As for security breaches, I added a few tests for eval-in-regex > > operators, or for interpolations with embedded code -- everything > > seems > > to be safe.

> > I think that's only because the code doesn't handle '{' in strings. > Even something simple like "WHERE foo = '{'" would fail. > > I cannot use quotemeta() as suggested, because we do want

> > special characters like \n or [aeiou] to have their usual meaning in > > regexes. For the same reason, we cannot use the single quote as > > delimiter, which would prevent any interpolation. > > > > I'm quite confident that no code insertion can be performed; but > > still > > there is the annoying fact that the regex could possibly contain > > interpolations of variables like $self, $0 or @main::ARGV, and > > unfortunately I don't see how to prevent that. Any suggestion ?

> > Code injection and interpolation are the same thing. Consider "WHERE > foo MATCH '$x[$y + system "rm -rf ~"]'". >

> > > > Anyway, such interpolations within regexes could cause exceptions; > > but > > I > > can't think of any security exploit based on this. So, since it's > > very > > unlikely to happen, and not very dangerous if it happens, there is no > > big risk.

Thanks for both. Confirmed they (especially the latter) cause serious issues. We definitely change this behavior, and add a note in the pod after we fix.

Tue Oct 28 04:52:51 2014 ishigaki [...] cpan.org - Correspondence added

RT-Send-CC:

laurent.dami [...] free.fr

On Tue Oct 28 12:53:46 2014, ISHIGAKI wrote: Show quoted text

> On Sun Oct 26 18:56:58 2014, MAUKE wrote:

> > On Sat Oct 25 15:23:38 2014, laurent.dami@free.fr wrote:

> > > Conditions for testing undef (NULL) have been added > > > (https://github.com/DBD-SQLite/DBD- > > > SQLite/commit/c6d9c27e22acb4235e2ba3e3dcf42631534cf054). > > > > > > As for security breaches, I added a few tests for eval-in-regex > > > operators, or for interpolations with embedded code -- everything > > > seems > > > to be safe.

> > > > I think that's only because the code doesn't handle '{' in strings. > > Even something simple like "WHERE foo = '{'" would fail. > > > > I cannot use quotemeta() as suggested, because we do want

> > > special characters like \n or [aeiou] to have their usual meaning > > > in > > > regexes. For the same reason, we cannot use the single quote as > > > delimiter, which would prevent any interpolation. > > > > > > I'm quite confident that no code insertion can be performed; but > > > still > > > there is the annoying fact that the regex could possibly contain > > > interpolations of variables like $self, $0 or @main::ARGV, and > > > unfortunately I don't see how to prevent that. Any suggestion ?

> > > > Code injection and interpolation are the same thing. Consider "WHERE > > foo MATCH '$x[$y + system "rm -rf ~"]'". > >

> > > > > > Anyway, such interpolations within regexes could cause exceptions; > > > but > > > I > > > can't think of any security exploit based on this. So, since it's > > > very > > > unlikely to happen, and not very dangerous if it happens, there is > > > no > > > big risk.

> > Thanks for both. Confirmed they (especially the latter) cause serious > issues. We definitely change this behavior, and add a note in the pod > after we fix.

Applied quotemeta and shipped 1.45_04 for testers.

Tue Oct 28 17:33:22 2014 DAMI [...] cpan.org - Correspondence added

On Mon Oct 27 23:53:46 2014, ISHIGAKI wrote: Show quoted text

> Thanks for both. Confirmed they (especially the latter) cause serious > issues.

Hi Kenichi, Did you actually try these code injections ? I did, and the code would not let them pass, so I'm interested to understand on which basis you confirmed that there was a serious issue. The addition of quotemeta() is not appropriate because it defeats the purpose of the MATCH operator : now if you write a query like "WHERE c MATCH '[rst]'", you don't get any result , because the square brackets are quoted and are no longer interpreted as a character class. For the same reason, you were obliged to tweak the test '^.i' ... but then it's no longer a regex ! Anyway, even if I still maintain that the implementation was safe, because the interpolation only happened in a very controlled environment, I nevertheless agree that an eval containing user-supplied strings is always scary, and I very well understand that security is a big concern for you as a release manager. But meanwhile I thought about this ticket and realized that there is no need for variable interpolation : the desired behaviour can be much better implemented through a closure, and then there is no risk of code injection (because Perl doesn't do double interpolation). So I propose the following : I will commit this new implementation in a separate branch, and add a few tests to make sure that the MATCH operator properly handles common regex idioms like character classes, anchors, etc. Then I'll leave it to you to merge that into trunk if you agree with the approach. Hope this will satisfy everybody.

Wed Oct 29 00:57:36 2014 ishigaki [...] cpan.org - Correspondence added

RT-Send-CC:

laurent.dami [...] free.fr

On Wed Oct 29 06:33:22 2014, DAMI wrote: Show quoted text

> On Mon Oct 27 23:53:46 2014, ISHIGAKI wrote: >

> > Thanks for both. Confirmed they (especially the latter) cause serious > > issues.

> > Hi Kenichi, > > Did you actually try these code injections ? I did, and the code would > not let them pass, so I'm interested to understand on which basis you > confirmed that there was a serious issue.

For the record for everyone reading this thread, I sent him a private email to show the exact code I used to confirm. Show quoted text

> > The addition of quotemeta() is not appropriate because it defeats the > purpose of the MATCH operator : now if you write a query like "WHERE c > MATCH '[rst]'", you don't get any result , because the square brackets > are quoted and are no longer interpreted as a character class. For the > same reason, you were obliged to tweak the test '^.i' ... but then > it's no longer a regex ! > > Anyway, even if I still maintain that the implementation was safe, > because the interpolation only happened in a very controlled > environment, I nevertheless agree that an eval containing user- > supplied strings is always scary, and I very well understand that > security is a big concern for you as a release manager. But meanwhile > I thought about this ticket and realized that there is no need for > variable interpolation : the desired behaviour can be much better > implemented through a closure, and then there is no risk of code > injection (because Perl doesn't do double interpolation). > > So I propose the following : I will commit this new implementation in > a separate branch, and add a few tests to make sure that the MATCH > operator properly handles common regex idioms like character classes, > anchors, etc. Then I'll leave it to you to merge that into trunk if > you agree with the approach.

Looking forward to it. Show quoted text

> > Hope this will satisfy everybody.

Thu Dec 11 23:48:59 2014 ishigaki [...] cpan.org - Correspondence added

Closed as DBD::SQLite 1.46 was released. Thanks!

Thu Dec 11 23:49:00 2014 ishigaki [...] cpan.org - Status changed from 'patched' to 'resolved'