Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

croak: CWE-134: Use of Externally-Controlled Format String #16108

Closed
p5pRT opened this issue Aug 10, 2017 · 13 comments
Closed

croak: CWE-134: Use of Externally-Controlled Format String #16108

p5pRT opened this issue Aug 10, 2017 · 13 comments

Comments

@p5pRT
Copy link

p5pRT commented Aug 10, 2017

Migrated from rt.perl.org#131878 (status was 'resolved')

Searchable as RT131878$

@p5pRT
Copy link
Author

p5pRT commented Aug 10, 2017

From @pali

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.

@p5pRT
Copy link
Author

p5pRT commented Aug 17, 2017

From @tonycoz

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.

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.

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.

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

@p5pRT
Copy link
Author

p5pRT commented Aug 17, 2017

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Aug 17, 2017

From @pali

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.

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.

@p5pRT
Copy link
Author

p5pRT commented Oct 11, 2017

From @pali

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?

@p5pRT
Copy link
Author

p5pRT commented Oct 12, 2017

From @tonycoz

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?

Tony

@p5pRT
Copy link
Author

p5pRT commented Oct 12, 2017

From @tonycoz

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

@p5pRT
Copy link
Author

p5pRT commented Oct 12, 2017

From @pali

On Wednesday 11 October 2017 20​:52​:15 Tony Cook via RT wrote​:

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.

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.

=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

@p5pRT
Copy link
Author

p5pRT commented Jan 17, 2018

From @tonycoz

On Thu, 12 Oct 2017 00​:58​:18 -0700, pali@​cpan.org wrote​:

-=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 8e14f28.

I've moved this ticket to the public queue.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jan 17, 2018

@tonycoz - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented Feb 1, 2018

From @pali

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.

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

@p5pRT
Copy link
Author

p5pRT commented Jun 23, 2018

From @khwilliamson

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.

@p5pRT
Copy link
Author

p5pRT commented Jun 23, 2018

@khwilliamson - Status changed from 'pending release' to 'resolved'

@p5pRT p5pRT closed this as completed Jun 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant