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

Unsafe decoding creates infinite loop #14

Closed
danaj opened this issue Oct 8, 2013 · 5 comments · Fixed by #15
Closed

Unsafe decoding creates infinite loop #14

danaj opened this issue Oct 8, 2013 · 5 comments · Fixed by #15
Assignees
Milestone

Comments

@danaj
Copy link
Contributor

danaj commented Oct 8, 2013

The following test of decoding unsafe input will make an infinite loop spewing warnings in 0.26:

use Convert::ASN1;
my $asn = Convert::ASN1->new;
$asn->prepare(q<
  [APPLICATION 7] SEQUENCE {
    int INTEGER
  }
>);
my $out;
$out = $asn->decode( pack("H*", "dfccd3fde3") );
$out = $asn->decode( pack("H*", "b0805f92cb") );

I ran random 5-byte strings to find two repeatable examples.

Fix: Add a position check to the two do loops on lines 636 and 690 of _decode.pm:

    do {
      $tag .= substr($_[0],$pos++,1);
      $b = ord substr($tag,-1);
    } while($b & 0x80 && $pos < $end);

This can happen in Convert::PEM when an incorrect password is used. See RT 27574 for an example.

@danaj
Copy link
Contributor Author

danaj commented Oct 9, 2013

Alternate fix. This seems to fit the existing style slightly better but I haven't seen any examples where it matters.

    do {
      return if $pos >= $end;
      $tag .= substr($_[0],$pos++,1);
      $b = ord substr($tag,-1);
    } while($b & 0x80);

This puts the test in front of the substr call so it happens before the first substr.

Also the "my $n = 1" at line 632 is unused.

I'll try to find time to do a pull request using the first code set.

@carnil
Copy link

carnil commented Apr 8, 2020

This issue seem to have CVE-2013-7488 assigned, cf. https://bugzilla.redhat.com/show_bug.cgi?id=1821879

@carnil
Copy link

carnil commented Apr 11, 2020

Mentioned corresponding pull request is at #15

gentoo-bot pushed a commit to gentoo/gentoo that referenced this issue Jun 28, 2020
- EAPI7
- Remove empty/unused variable assignments
- Add patch submitted to upstream repo to remedy CVE-2013-7488

Bug: https://bugs.gentoo.org/716680
Bug: gbarr/perl-Convert-ASN1#15
Bug: gbarr/perl-Convert-ASN1#14
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1821879
Package-Manager: Portage-2.3.100, Repoman-2.3.22
Signed-off-by: Kent Fredric <kentnl@gentoo.org>
NeddySeagoon pushed a commit to NeddySeagoon/gentoo-arm64 that referenced this issue Jun 29, 2020
- EAPI7
- Remove empty/unused variable assignments
- Add patch submitted to upstream repo to remedy CVE-2013-7488

Bug: https://bugs.gentoo.org/716680
Bug: gbarr/perl-Convert-ASN1#15
Bug: gbarr/perl-Convert-ASN1#14
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1821879
Package-Manager: Portage-2.3.100, Repoman-2.3.22
Signed-off-by: Kent Fredric <kentnl@gentoo.org>
@carnil
Copy link

carnil commented Mar 1, 2021

@gbarr do the proposed change look good to be merged?

@gbarr
Copy link
Owner

gbarr commented Mar 6, 2021

@carnil I have not been active with anything Perl for a long time. If anyone wants to take maintainership I would be happy to pass it on

@timlegge timlegge self-assigned this May 22, 2021
@timlegge timlegge added this to the 0.28-TRIAL milestone May 22, 2021
@timlegge timlegge linked a pull request May 22, 2021 that will close this issue
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

Successfully merging a pull request may close this issue.

4 participants