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

Strict octal checking seems to be too strict #2213

Open
spion06 opened this Issue Feb 26, 2018 · 14 comments

Comments

Projects
None yet
8 participants
@spion06

spion06 commented Feb 26, 2018

I'm currently seeing issues when extracting tar archives after the changes made on 92e98bf

I receive the following error when running rubygems 2.7.6. I do not see error running 2.7.5 and the tar CLI does not show any issues with the archive.

https://gist.github.com/spion06/611d0593f893e5c0e9a4bd274c808a0c#file-tests-md

This issue is related to:

  • Network problems
  • Installing a library
  • Publishing a library
  • The command line gem
  • Other

Here are my current environment details:

$ gem env version
2.7.6

I will abide by the code of conduct.

@spion06

This comment has been minimized.

Show comment
Hide comment
@spion06

spion06 Feb 26, 2018

I was able to narrow it down to the UID/GID fields being what is causing the issue here.

spion06 commented Feb 26, 2018

I was able to narrow it down to the UID/GID fields being what is causing the issue here.

@spion06

This comment has been minimized.

Show comment
Hide comment
@spion06

spion06 Feb 26, 2018

specifically this occurs when when the UID or GID exceed the value of 16777215 (or 8^8). Which would appear to make sense give the tar spec lays out that the UID/GID fields should be 8 octal bytes of data. It seems that is not always the case though

https://www.gnu.org/software/tar/manual/html_section/tar_92.html

spion06 commented Feb 26, 2018

specifically this occurs when when the UID or GID exceed the value of 16777215 (or 8^8). Which would appear to make sense give the tar spec lays out that the UID/GID fields should be 8 octal bytes of data. It seems that is not always the case though

https://www.gnu.org/software/tar/manual/html_section/tar_92.html

@bronzdoc

This comment has been minimized.

Show comment
Hide comment
@bronzdoc

bronzdoc Feb 26, 2018

Member

I can reproduce with the latest rubygems version, thanks for the example

Member

bronzdoc commented Feb 26, 2018

I can reproduce with the latest rubygems version, thanks for the example

@spion06

This comment has been minimized.

Show comment
Hide comment
@spion06

spion06 Feb 26, 2018

The pax header format could be what is causing this problem

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/pax.html#tag_20_92_13_03

doing some local tests i'm able to re-produce it pretty easily when tar decides to use this format.

spion06 commented Feb 26, 2018

The pax header format could be what is causing this problem

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/pax.html#tag_20_92_13_03

doing some local tests i'm able to re-produce it pretty easily when tar decides to use this format.

jeffbyrnes added a commit to evertrue/exhibitor-cookbook that referenced this issue Apr 9, 2018

jeffbyrnes added a commit to evertrue/exhibitor-cookbook that referenced this issue Apr 9, 2018

jeffbyrnes added a commit to evertrue/exhibitor-cookbook that referenced this issue Apr 9, 2018

jeffbyrnes added a commit to evertrue/exhibitor-cookbook that referenced this issue Apr 9, 2018

jeffbyrnes added a commit to evertrue/exhibitor-cookbook that referenced this issue Apr 9, 2018

@jeffbyrnes

This comment has been minimized.

Show comment
Hide comment
@jeffbyrnes

jeffbyrnes Apr 9, 2018

Sorry for the reference spam! I was debugging something on Travis, and it required some amending.

jeffbyrnes commented Apr 9, 2018

Sorry for the reference spam! I was debugging something on Travis, and it required some amending.

jeffbyrnes added a commit to darkskyapp/cdo-cookbook that referenced this issue Apr 10, 2018

@jkutner

This comment has been minimized.

Show comment
Hide comment
@jkutner

jkutner May 4, 2018

Steps to reproduce:

$ tar --version
tar (GNU tar) 1.29
Copyright (C) 2015 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by John Gilmore and Jay Fenlason.
$ echo "hello" > foo.txt
$ tar czf test.tgz foo.txt
$ ruby -rrubygems -e "require 'rubygems/package'; Gem::Package::TarReader.new(StringIO.new(Zlib::GzipReader.new(File.new('test.tgz')).read)) {|t| puts t.each {|f| f.inspect}}"
Traceback (most recent call last):
        5: from -e:1:in `<main>'
        4: from /Users/jkutner/.rvm/rubies/ruby-2.5.1/lib/ruby/2.5.0/rubygems/package/tar_reader.rb:29:in `new'
        3: from -e:1:in `block in <main>'
        2: from /Users/jkutner/.rvm/rubies/ruby-2.5.1/lib/ruby/2.5.0/rubygems/package/tar_reader.rb:59:in `each'
        1: from /Users/jkutner/.rvm/rubies/ruby-2.5.1/lib/ruby/2.5.0/rubygems/package/tar_header.rb:108:in `from'
/Users/jkutner/.rvm/rubies/ruby-2.5.1/lib/ruby/2.5.0/rubygems/package/tar_header.rb:128:in `strict_oct': "\x80\x00\x00\x00\x16\x98\x02r" is not an octal string (ArgumentError)

Also:

$ uname -a
Darwin xxx 17.5.0 Darwin Kernel Version 17.5.0: Fri Apr 13 19:32:32 PDT 2018; root:xnu-4570.51.2~1/RELEASE_X86_64 x86_64

jkutner commented May 4, 2018

Steps to reproduce:

$ tar --version
tar (GNU tar) 1.29
Copyright (C) 2015 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by John Gilmore and Jay Fenlason.
$ echo "hello" > foo.txt
$ tar czf test.tgz foo.txt
$ ruby -rrubygems -e "require 'rubygems/package'; Gem::Package::TarReader.new(StringIO.new(Zlib::GzipReader.new(File.new('test.tgz')).read)) {|t| puts t.each {|f| f.inspect}}"
Traceback (most recent call last):
        5: from -e:1:in `<main>'
        4: from /Users/jkutner/.rvm/rubies/ruby-2.5.1/lib/ruby/2.5.0/rubygems/package/tar_reader.rb:29:in `new'
        3: from -e:1:in `block in <main>'
        2: from /Users/jkutner/.rvm/rubies/ruby-2.5.1/lib/ruby/2.5.0/rubygems/package/tar_reader.rb:59:in `each'
        1: from /Users/jkutner/.rvm/rubies/ruby-2.5.1/lib/ruby/2.5.0/rubygems/package/tar_header.rb:108:in `from'
/Users/jkutner/.rvm/rubies/ruby-2.5.1/lib/ruby/2.5.0/rubygems/package/tar_header.rb:128:in `strict_oct': "\x80\x00\x00\x00\x16\x98\x02r" is not an octal string (ArgumentError)

Also:

$ uname -a
Darwin xxx 17.5.0 Darwin Kernel Version 17.5.0: Fri Apr 13 19:32:32 PDT 2018; root:xnu-4570.51.2~1/RELEASE_X86_64 x86_64
@drbrain

This comment has been minimized.

Show comment
Hide comment
@drbrain

drbrain May 25, 2018

Member

92e98bf is the culprit, but doesn't say anything about what tar files caused this issue.

In the "Other Extensions" section of the BSD tar man page notes that these are valid tar files and the change in the above commit is incomplete as it doesn't allow these valid values through.

@segiddins can you shed any light on this? Since this change RubyGems can't unpack tar files as used by chef and others, and there's no link to any PR nor issue that explains any further, nor any approval.

Member

drbrain commented May 25, 2018

92e98bf is the culprit, but doesn't say anything about what tar files caused this issue.

In the "Other Extensions" section of the BSD tar man page notes that these are valid tar files and the change in the above commit is incomplete as it doesn't allow these valid values through.

@segiddins can you shed any light on this? Since this change RubyGems can't unpack tar files as used by chef and others, and there's no link to any PR nor issue that explains any further, nor any approval.

@segiddins

This comment has been minimized.

Show comment
Hide comment
@segiddins

segiddins May 25, 2018

Member

It was fixing a security vulnerability that was fixed in the private repo, hence the lack of a PR link. As the commit message says, tar files that had octal fields set to -... would return negative numbers from String#oct, causing gem commands to loop infinitely. It was approved by @claudijd, in response to a HackerOne report by plover.

Member

segiddins commented May 25, 2018

It was fixing a security vulnerability that was fixed in the private repo, hence the lack of a PR link. As the commit message says, tar files that had octal fields set to -... would return negative numbers from String#oct, causing gem commands to loop infinitely. It was approved by @claudijd, in response to a HackerOne report by plover.

@drbrain

This comment has been minimized.

Show comment
Hide comment
@drbrain

drbrain May 25, 2018

Member

Setting which fields -… cause this behavior?

Tar files can contain fields that the ustar format says are "octal" but contain other formats, including fields that start with a - for some rare tar files. See gnutar from_header and libarchive validate_number_field

Without documenting which fields cause the original bug we can't evaluate fixing the original bug correctly.

Member

drbrain commented May 25, 2018

Setting which fields -… cause this behavior?

Tar files can contain fields that the ustar format says are "octal" but contain other formats, including fields that start with a - for some rare tar files. See gnutar from_header and libarchive validate_number_field

Without documenting which fields cause the original bug we can't evaluate fixing the original bug correctly.

@segiddins

This comment has been minimized.

Show comment
Hide comment
@segiddins

segiddins May 25, 2018

Member

It's whatever was attached to the report, sorry I don't remember off the top of my head

Member

segiddins commented May 25, 2018

It's whatever was attached to the report, sorry I don't remember off the top of my head

@drbrain

This comment has been minimized.

Show comment
Hide comment
@drbrain

drbrain May 25, 2018

Member

Can you give a link to the report? It's not included in the commit

Member

drbrain commented May 25, 2018

Can you give a link to the report? It's not included in the commit

@hone

This comment has been minimized.

Show comment
Hide comment
@hone

hone May 25, 2018

Contributor

@drbrain I think this is the report https://hackerone.com/reports/281336 that @segiddins linked above.

Contributor

hone commented May 25, 2018

@drbrain I think this is the report https://hackerone.com/reports/281336 that @segiddins linked above.

@drbrain

This comment has been minimized.

Show comment
Hide comment
@drbrain

drbrain May 25, 2018

Member

Ah, I think the link was added in an edit, so I missed it, thanks

Member

drbrain commented May 25, 2018

Ah, I think the link was added in an edit, so I missed it, thanks

@havocp

This comment has been minimized.

Show comment
Hide comment
@havocp

havocp Aug 24, 2018

The cases of this I've seen (and also in the pasted error above) are fields starting with \x80 which flags a positive base-256 number. A negative number would have 0xff I guess but I haven't run into that. Check out tar_atol in BSD tar here: https://opensource.apple.com/source/libarchive/libarchive-41.20.1/libarchive/libarchive/archive_read_support_format_tar.c.auto.html

havocp commented Aug 24, 2018

The cases of this I've seen (and also in the pasted error above) are fields starting with \x80 which flags a positive base-256 number. A negative number would have 0xff I guess but I haven't run into that. Check out tar_atol in BSD tar here: https://opensource.apple.com/source/libarchive/libarchive-41.20.1/libarchive/libarchive/archive_read_support_format_tar.c.auto.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment