Skip Menu |
Report information
Id: 131878
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: pali [at] cpan.org
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: (no value)
Type: (no value)
Perl Version: (no value)
Fixed In: (no value)

Attachments
0001-perl-131878-don-t-call-croak-with-a-potential-format.patch



From: pali [...] cpan.org
Subject: croak: CWE-134: Use of Externally-Controlled Format String
To: perl5-security-report [...] perl.org
Date: Thu, 10 Aug 2017 14:13:45 +0200
Download (untitled) / with headers
text/plain 815b
Hi! In perlblead there are at lest 3 places where arbitrary string supplied by caller can be passed into Perl_croak function which expects printf-style arguments. One is in eval_pv() implementation from cpan/Devel-PPPort/parts/inc/call second in Socket.xs and third is in documentation sample for my_eval_sv in perlembed.pod. Probably there are also other places, but I have not looked deeply. In all three places is printf-style format argument taken from ERRSV, $@ variable which can contain arbitrary string set by user or also by remote system. E.g. when remote database server throw error, DBI driver propagate this error (or part of it) via $@ to caller. Malicious remote system via specially crafted error message then can cause problems like buffer overflow or overwriting other part of process memory.
RT-Send-CC: rt-deliver-to-perl5-security-report [...] rt.perl.org
Download (untitled) / with headers
text/plain 1.2k
On Thu, 10 Aug 2017 05:14:10 -0700, pali@cpan.org wrote: Show quoted text
> Hi! In perlblead there are at lest 3 places where arbitrary string > supplied by caller can be passed into Perl_croak function which expects > printf-style arguments. > > One is in eval_pv() implementation from cpan/Devel-PPPort/parts/inc/call
Found this one. I believe this should be reported upstream. Show quoted text
> second in Socket.xs
I couldn't find this. Socket.xs defines a bad croak_sv() macro but doesn't use it. Can you point me at the line? In any case, it should be reported upstream. Show quoted text
> and third is in documentation sample for my_eval_sv > in perlembed.pod.
Found this one. This in itself isn't a security issue, but it should be fixed. Show quoted text
> > Probably there are also other places, but I have not looked deeply. > > In all three places is printf-style format argument taken from ERRSV, $@ > variable which can contain arbitrary string set by user or also by > remote system. E.g. when remote database server throw error, DBI driver > propagate this error (or part of it) via $@ to caller. > > Malicious remote system via specially crafted error message then can > cause problems like buffer overflow or overwriting other part of process > memory.
If you find any others, please let us know. Tony
Date: Thu, 17 Aug 2017 09:19:09 +0200
To: perl5-security-report [...] perl.org
Subject: Re: [perl #131878] croak: CWE-134: Use of Externally-Controlled Format String
From: pali [...] cpan.org
Download (untitled) / with headers
text/plain 2.3k
Hi! On Wednesday 16 August 2017 17:41:36 Tony Cook via RT wrote: Show quoted text
> On Thu, 10 Aug 2017 05:14:10 -0700, pali@cpan.org wrote:
> > Hi! In perlblead there are at lest 3 places where arbitrary string > > supplied by caller can be passed into Perl_croak function which expects > > printf-style arguments. > > > > One is in eval_pv() implementation from cpan/Devel-PPPort/parts/inc/call
> > Found this one. > > I believe this should be reported upstream.
I already did it few months ago, but nothing happened. Report was fully ignored and because perl distribution bundle this part, it should be fixed also there. Upstream seems dead now. Show quoted text
> > second in Socket.xs
> > I couldn't find this. Socket.xs defines a bad croak_sv() macro but doesn't use it. > > Can you point me at the line?
Right, it is the wrong croak_sv(). Even it is not used, it is a problem as in future somebody could start using this defined function/macro. Show quoted text
> In any case, it should be reported upstream.
Ok, I will try, but after Devel-PPPort I'm sceptic about reporting it. Show quoted text
> > and third is in documentation sample for my_eval_sv > > in perlembed.pod.
> > Found this one. This in itself isn't a security issue, but it should be fixed.
As this provides basically fully working example program which is going to be used by people who read that documentation, it is a security issue for those people. So it should be fixed and possible in some announcement should be written that people who used old version of this example should check if they copied vulnerable code and fix them appropriately. Show quoted text
> > > > Probably there are also other places, but I have not looked deeply. > > > > In all three places is printf-style format argument taken from ERRSV, $@ > > variable which can contain arbitrary string set by user or also by > > remote system. E.g. when remote database server throw error, DBI driver > > propagate this error (or part of it) via $@ to caller. > > > > Malicious remote system via specially crafted error message then can > > cause problems like buffer overflow or overwriting other part of process > > memory.
> > If you find any others, please let us know. > > Tony
Probably the best would be if you define croak_sv, warn_sv and other macros in perl's Devel-PPPort bundled code properly, so other people do not have to write their own possible vulnerable implementation. And would use just one which could be properly verified by perl p5p team.
To: perl5-security-report [...] perl.org
From: pali [...] cpan.org
Subject: Re: [perl #131878] croak: CWE-134: Use of Externally-Controlled Format String
Date: Wed, 11 Oct 2017 14:38:42 +0200
Download (untitled) / with headers
text/plain 1.7k
On Thursday 17 August 2017 09:19:09 pali@cpan.org wrote: Show quoted text
> Hi! > > On Wednesday 16 August 2017 17:41:36 Tony Cook via RT wrote:
> > On Thu, 10 Aug 2017 05:14:10 -0700, pali@cpan.org wrote:
> > > Hi! In perlblead there are at lest 3 places where arbitrary string > > > supplied by caller can be passed into Perl_croak function which expects > > > printf-style arguments. > > > > > > One is in eval_pv() implementation from cpan/Devel-PPPort/parts/inc/call
> > > > Found this one. > > > > I believe this should be reported upstream.
> > I already did it few months ago, but nothing happened. Report was fully > ignored and because perl distribution bundle this part, it should be > fixed also there. Upstream seems dead now. >
> > > second in Socket.xs
> > > > I couldn't find this. Socket.xs defines a bad croak_sv() macro but doesn't use it. > > > > Can you point me at the line?
> > Right, it is the wrong croak_sv(). Even it is not used, it is a problem > as in future somebody could start using this defined function/macro. >
> > In any case, it should be reported upstream.
> > Ok, I will try, but after Devel-PPPort I'm sceptic about reporting it. >
> > > and third is in documentation sample for my_eval_sv > > > in perlembed.pod.
> > > > Found this one. This in itself isn't a security issue, but it should be fixed.
> > As this provides basically fully working example program which is going > to be used by people who read that documentation, it is a security issue > for those people. > > So it should be fixed and possible in some announcement should be > written that people who used old version of this example should check if > they copied vulnerable code and fix them appropriately.
PING! What is state of this issue? When it will be fixed in perl itself?
RT-Send-CC: perl5-security-report [...] perl.org
On Wed, 11 Oct 2017 05:39:01 -0700, pali@cpan.org wrote: Show quoted text
> On Thursday 17 August 2017 09:19:09 pali@cpan.org wrote:
> > Hi! > > > > On Wednesday 16 August 2017 17:41:36 Tony Cook via RT wrote:
> > > On Thu, 10 Aug 2017 05:14:10 -0700, pali@cpan.org wrote:
> > > > Hi! In perlblead there are at lest 3 places where arbitrary > > > > string > > > > supplied by caller can be passed into Perl_croak function which > > > > expects > > > > printf-style arguments. > > > > > > > > One is in eval_pv() implementation from cpan/Devel- > > > > PPPort/parts/inc/call
> > > > > > Found this one. > > > > > > I believe this should be reported upstream.
> > > > I already did it few months ago, but nothing happened. Report was > > fully > > ignored and because perl distribution bundle this part, it should be > > fixed also there. Upstream seems dead now. > >
> > > > second in Socket.xs
> > > > > > I couldn't find this. Socket.xs defines a bad croak_sv() macro but > > > doesn't use it. > > > > > > Can you point me at the line?
> > > > Right, it is the wrong croak_sv(). Even it is not used, it is a > > problem > > as in future somebody could start using this defined function/macro. > >
> > > In any case, it should be reported upstream.
> > > > Ok, I will try, but after Devel-PPPort I'm sceptic about reporting > > it. > >
> > > > and third is in documentation sample for my_eval_sv > > > > in perlembed.pod.
> > > > > > Found this one. This in itself isn't a security issue, but it > > > should be fixed.
> > > > As this provides basically fully working example program which is > > going > > to be used by people who read that documentation, it is a security > > issue > > for those people. > > > > So it should be fixed and possible in some announcement should be > > written that people who used old version of this example should check > > if > > they copied vulnerable code and fix them appropriately.
> > PING! What is state of this issue? When it will be fixed in perl > itself?
Is the attached something like what you're looking for as a change? Tony
Subject: 0001-perl-131878-don-t-call-croak-with-a-potential-format.patch
From 323c22704dbc842cb967d875e41090b3606064b3 Mon Sep 17 00:00:00 2001 From: Tony Cook <tony@develop-help.com> Date: Thu, 12 Oct 2017 14:51:23 +1100 Subject: (perl #131878) don't call croak() with a potential format string --- pod/perldelta.pod | 12 ++++++++++-- pod/perlembed.pod | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/pod/perldelta.pod b/pod/perldelta.pod index c0beb06..fbab8ef 100644 --- a/pod/perldelta.pod +++ b/pod/perldelta.pod @@ -165,13 +165,21 @@ section. Additionally, the following selected changes have been made: -=head3 L<XXX> +=head3 L<perlembed> =over 4 =item * -XXX Description of the change here +An example in L<perlembed> used the string value of C<ERRSV> as a +format string when calling croak(). If that string contains format +codes such as C<%s> this could crash the program. + +This has been changed to a call to croak_sv(). + +An alternative could have been to supply a trivial format string: + + croak("%s", SvPV_nolen(ERRSV)); =back diff --git a/pod/perlembed.pod b/pod/perlembed.pod index 70f8e0d..91759a3 100644 --- a/pod/perlembed.pod +++ b/pod/perlembed.pod @@ -429,7 +429,7 @@ been wrapped here): PUTBACK; if (croak_on_error && SvTRUE(ERRSV)) - croak(SvPVx_nolen(ERRSV)); + croak_sv(ERRSV); return retval; } -- 2.1.4
From: pali [...] cpan.org
Subject: Re: [perl #131878] croak: CWE-134: Use of Externally-Controlled Format String
Date: Thu, 12 Oct 2017 09:57:53 +0200
To: perl5-security-report-followup [...] perl.org
Download (untitled) / with headers
text/plain 3.8k
On Wednesday 11 October 2017 20:52:15 Tony Cook via RT wrote: Show quoted text
> On Wed, 11 Oct 2017 05:39:01 -0700, pali@cpan.org wrote:
> > On Thursday 17 August 2017 09:19:09 pali@cpan.org wrote:
> > > Hi! > > > > > > On Wednesday 16 August 2017 17:41:36 Tony Cook via RT wrote:
> > > > On Thu, 10 Aug 2017 05:14:10 -0700, pali@cpan.org wrote:
> > > > > Hi! In perlblead there are at lest 3 places where arbitrary > > > > > string > > > > > supplied by caller can be passed into Perl_croak function which > > > > > expects > > > > > printf-style arguments. > > > > > > > > > > One is in eval_pv() implementation from cpan/Devel- > > > > > PPPort/parts/inc/call
> > > > > > > > Found this one. > > > > > > > > I believe this should be reported upstream.
> > > > > > I already did it few months ago, but nothing happened. Report was > > > fully > > > ignored and because perl distribution bundle this part, it should be > > > fixed also there. Upstream seems dead now. > > >
> > > > > second in Socket.xs
> > > > > > > > I couldn't find this. Socket.xs defines a bad croak_sv() macro but > > > > doesn't use it. > > > > > > > > Can you point me at the line?
> > > > > > Right, it is the wrong croak_sv(). Even it is not used, it is a > > > problem > > > as in future somebody could start using this defined function/macro. > > >
> > > > In any case, it should be reported upstream.
> > > > > > Ok, I will try, but after Devel-PPPort I'm sceptic about reporting > > > it. > > >
> > > > > and third is in documentation sample for my_eval_sv > > > > > in perlembed.pod.
> > > > > > > > Found this one. This in itself isn't a security issue, but it > > > > should be fixed.
> > > > > > As this provides basically fully working example program which is > > > going > > > to be used by people who read that documentation, it is a security > > > issue > > > for those people. > > > > > > So it should be fixed and possible in some announcement should be > > > written that people who used old version of this example should check > > > if > > > they copied vulnerable code and fix them appropriately.
> > > > PING! What is state of this issue? When it will be fixed in perl > > itself?
> > Is the attached something like what you're looking for as a change?
Yes. Removing all vulnerable chunks from the both documentation and perl source code itself. Show quoted text
> From 323c22704dbc842cb967d875e41090b3606064b3 Mon Sep 17 00:00:00 2001 > From: Tony Cook <tony@develop-help.com> > Date: Thu, 12 Oct 2017 14:51:23 +1100 > Subject: (perl #131878) don't call croak() with a potential format string > > --- > pod/perldelta.pod | 12 ++++++++++-- > pod/perlembed.pod | 2 +- > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/pod/perldelta.pod b/pod/perldelta.pod > index c0beb06..fbab8ef 100644 > --- a/pod/perldelta.pod > +++ b/pod/perldelta.pod > @@ -165,13 +165,21 @@ section. > > Additionally, the following selected changes have been made: > > -=head3 L<XXX> > +=head3 L<perlembed> > > =over 4 > > =item * > > -XXX Description of the change here > +An example in L<perlembed> used the string value of C<ERRSV> as a > +format string when calling croak(). If that string contains format > +codes such as C<%s> this could crash the program. > + > +This has been changed to a call to croak_sv(). > + > +An alternative could have been to supply a trivial format string: > + > + croak("%s", SvPV_nolen(ERRSV));
Or in case ERRSV needs to be passed into croak, then croak(NULL) can be used since Perl 5.6.1. As it preserves object exceptions or UTF-8 flag in ERRSV. Show quoted text
> > =back > > diff --git a/pod/perlembed.pod b/pod/perlembed.pod > index 70f8e0d..91759a3 100644 > --- a/pod/perlembed.pod > +++ b/pod/perlembed.pod > @@ -429,7 +429,7 @@ been wrapped here): > PUTBACK; > > if (croak_on_error && SvTRUE(ERRSV)) > - croak(SvPVx_nolen(ERRSV)); > + croak_sv(ERRSV); > > return retval; > } > -- > 2.1.4 >
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 894b
On Thu, 12 Oct 2017 00:58:18 -0700, pali@cpan.org wrote: Show quoted text
> > -=head3 L<XXX> > > +=head3 L<perlembed> > > > > =over 4 > > > > =item * > > > > -XXX Description of the change here > > +An example in L<perlembed> used the string value of C<ERRSV> as a > > +format string when calling croak(). If that string contains format > > +codes such as C<%s> this could crash the program. > > + > > +This has been changed to a call to croak_sv(). > > + > > +An alternative could have been to supply a trivial format string: > > + > > + croak("%s", SvPV_nolen(ERRSV));
> > Or in case ERRSV needs to be passed into croak, then croak(NULL) can be > used since Perl 5.6.1. As it preserves object exceptions or UTF-8 flag > in ERRSV.
I've applied my patch with an added mention of croak(NULL) in perldelta as 8e14f284e9d93dc3a96622cee4ebdc135e677581. I've moved this ticket to the public queue. Tony
To: perl5-security-report [...] perl.org
Subject: Re: [perl #131878] croak: CWE-134: Use of Externally-Controlled Format String
From: pali [...] cpan.org
Date: Thu, 1 Feb 2018 10:57:51 +0100
Download (untitled) / with headers
text/plain 900b
On Thursday 17 August 2017 09:19:09 pali@cpan.org wrote: Show quoted text
> Hi! > > On Wednesday 16 August 2017 17:41:36 Tony Cook via RT wrote:
> > On Thu, 10 Aug 2017 05:14:10 -0700, pali@cpan.org wrote:
> > > Hi! In perlblead there are at lest 3 places where arbitrary string > > > supplied by caller can be passed into Perl_croak function which expects > > > printf-style arguments. > > > > > > One is in eval_pv() implementation from cpan/Devel-PPPort/parts/inc/call
> > > > Found this one. > > > > I believe this should be reported upstream.
> > I already did it few months ago, but nothing happened. Report was fully > ignored and because perl distribution bundle this part, it should be > fixed also there. Upstream seems dead now.
Devel::PPPort was moved from cpan/ to dist/ and this problem was fixed in perl commit: https://perl5.git.perl.org/perl.git/commit/7ceac2e89b40347b95855c622b9f8f31bd6560f4
Download (untitled) / with headers
text/plain 317b
Thank you for filing this report. You have helped make Perl better. With the release yesterday of Perl 5.28.0, this and 185 other issues have been resolved. Perl 5.28.0 may be downloaded via: https://metacpan.org/release/XSAWYERX/perl-5.28.0 If you find that the problem persists, feel free to reopen this ticket.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org