Bug 1586295 - Review Request: rubygem-bootsnap - Boot large ruby/rails apps faster
Summary: Review Request: rubygem-bootsnap - Boot large ruby/rails apps faster
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jun Aruga
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-06-05 23:10 UTC by Pavel Valena
Modified: 2018-06-15 16:19 UTC (History)
3 users (show)

Fixed In Version: rubygem-bootsnap-1.3.0-1.fc29
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-06-15 16:19:59 UTC
jaruga: fedora-review+


Attachments (Terms of Use)

Description Pavel Valena 2018-06-05 23:10:45 UTC
Spec URL: https://pvalena.fedorapeople.org/gems/rubygem-bootsnap.spec
SRPM URL: https://pvalena.fedorapeople.org/gems/rubygem-bootsnap-1.3.0-1.fc29.src.rpm
Description: Bootsnap is a library that plugs into Ruby, with optional support for ActiveSupport and YAML, to optimize and cache expensive computations.

Fedora Account System Username: pvalena
Koji scratch-build: https://koji.fedoraproject.org/koji/taskinfo?taskID=27444434

Comment 1 Jun Aruga 2018-06-07 16:33:22 UTC
First of all, your unit test is actually executed for zero files.
As the test files (test/*) are not included in the gem file, you have to prepare the archive file including those files.

Comment 2 Jun Aruga 2018-06-07 16:40:37 UTC
I guess as below files are used for development, those should be included in %files doc

```
bin/console
bin/setup
bin/testunit
```

Comment 3 Pavel Valena 2018-06-07 23:33:34 UTC
(In reply to Jun Aruga from comment #1)
> First of all, your unit test is actually executed for zero files.
> As the test files (test/*) are not included in the gem file, you have to
> prepare the archive file including those files.

Hmm, strange, I thought I've checked and fixed this.

(In reply to Jun Aruga from comment #2)
> I guess as below files are used for development, those should be included in
> %files doc
> 
> ```
> bin/console
> bin/setup
> bin/testunit
> ```

True.

I've done the fixes and re-uploaded srpm and .spec file (links in Description).

Scratch-builds (ARM si failing -reproducibly- for some reason):
  https://koji.fedoraproject.org/koji/taskinfo?taskID=27478103
  https://koji.fedoraproject.org/koji/taskinfo?taskID=27478502
  https://koji.fedoraproject.org/koji/taskinfo?taskID=27478541
  https://koji.fedoraproject.org/koji/taskinfo?taskID=27478650
  https://koji.fedoraproject.org/koji/taskinfo?taskID=27478585
  https://koji.fedoraproject.org/koji/taskinfo?taskID=27478676

Comment 4 Jun Aruga 2018-06-08 12:45:53 UTC
I am fine to skip the test failure this time for armv7hl.

```
%ifarch armv7hl
...
ruby -Ilib:test:ext -e 'Dir.glob "./test/**/*_test.rb", &method(:require)'
...
%endif
```

Comment 5 Jun Aruga 2018-06-08 13:02:06 UTC
You need source contribution to the upstream to change the logic, don't you?

```
$ rpmlint -i ./*.spec /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm
...
rubygem-bootsnap.x86_64: E: call-to-mktemp /usr/lib64/gems/ruby/bootsnap-1.3.0/bootsnap/bootsnap.so
This executable calls mktemp. As advised by the manpage (mktemp(3)), this
function should be avoided. Some implementations are deeply insecure, and 
there is a race condition between the time of check and time of use (TOCTOU).
See http://capec.mitre.org/data/definitions/29.html for details, and contact
upstream to have this issue fixed.
```

Comment 6 Jun Aruga 2018-06-08 13:50:38 UTC
You can move CODE_OF_CONDUCT.md to %files doc?

Other things look okay!

Comment 7 Pavel Valena 2018-06-08 15:28:42 UTC
> I am fine to skip the test failure this time for armv7hl.

I'll file a bug when the component is created.
In the meantime I'll exclude the armv7hl arch, as it does not work at all.

> You can move CODE_OF_CONDUCT.md to %files doc?

Sure, I somehow overlooked it...

> Other things look okay!

I do not think wee need to do anything with the `call-to-mktemp` - see below.

Reading the code:
 * On C level [1] - expects atomic access to a file (used for caching only, reasonable hashing method is used); and
 * On Ruby level [2] - Mutex is used for synchronizing the Threads

Furthermore, reading CAPEC[3], as suggested by rpmlint, none of the Attack Prerequisites are not met AFAICT.
This gem is also heavily used and developed at Shopify (and now enabled by default in any Rails app). Concluding that any security concerns one might have have been very probably already investigated.

[1] https://github.com/Shopify/bootsnap/blob/684acfd9b8c1298a026dd6b9c2ffeb173d11e949/ext/bootsnap/bootsnap.c#L466
[2] https://github.com/Shopify/bootsnap/blob/684acfd9b8c1298a026dd6b9c2ffeb173d11e949/lib/bootsnap/load_path_cache/cache.rb#L11
[3] http://capec.mitre.org/data/definitions/29.html

Additionally, I've commented out any $CFLAGS modification in `extconf.rb` file to use the default Fedora ones.

I've updated the links again, Scratch-build:
  https://koji.fedoraproject.org/koji/taskinfo?taskID=27490538

Thanks for the review!

Comment 8 Jun Aruga 2018-06-11 09:45:51 UTC
> I do not think wee need to do anything with the `call-to-mktemp` - see below.
> 
> Reading the code:
>  * On C level [1] - expects atomic access to a file (used for caching only, reasonable hashing method is used); and
>  * On Ruby level [2] - Mutex is used for synchronizing the Threads

Are you sure?
It seems that the method "atomic_write_cache_file" which mktemp is called in is expecting as a attomic.
But seeing the actual logic, I think the logic [2] is not related to the called method atomic_write_cache_file. As you know, Mutex itself is used for native thread. But it does not guarantee atomic access if lock is not used. It depends on the implementation.

lib/bootsnap.rb Bootsnap::CompileCache.setup
-> lib/bootsnap/compile_cache.rb Bootsnap::CompileCache.setup.
  -> lib/bootsnap/compile_cache/iseq.rb: Bootsnap::CompileCache::Native.fetch def self.install!(cache_dir)
  -> lib/bootsnap/compile_cache/yaml.rb: Bootsnap::CompileCache::Native.fetch def self.install!(cache_dir)
    -> ext/bootsnap/bootsnap.c
      rb_define_module_function(rb_mBootsnap_CompileCache_Native, "fetch", bs_rb_fetch, 3);
      bs_rb_fetch -> bs_fetch -> 
      atomic_write_cache_file <= it looks atomic access from the method name.
          tmp_path = mktemp(template);

For example it seems that when below logic is called several times at same timing, it does not guarantee the atomic access.

```
irb(main):001:0> require 'bootsnap'
irb(main):006:0> Bootsnap.setup(cache_dir: '/tmp/foo', autoload_paths_cache: false)
=> :load_file
```

Comment 9 Jun Aruga 2018-06-11 09:57:05 UTC
Ref: How to attack is written here: https://en.wikipedia.org/wiki/Time_of_check_to_time_of_use

Comment 10 Jun Aruga 2018-06-11 10:25:02 UTC
How about using mkstemp instead of mktemp? Though I am not confident for that? or asking security guys?

https://www.owasp.org/index.php/Insecure_Temporary_File
> Finally, mkstemp() is a reasonably safe way to create temporary files.

Comment 11 Jun Aruga 2018-06-11 10:37:13 UTC
> I'll file a bug when the component is created.
> In the meantime I'll exclude the armv7hl arch, as it does not work at all.

Okay. It is good to file the bug as Bugzilla.

> ExcludeArch: armv7hl 

Okay, this looks better.

Other things look good to me.

Comment 12 Pavel Valena 2018-06-11 11:51:10 UTC
(In reply to Jun Aruga from comment #10)
> How about using mkstemp instead of mktemp? Though I am not confident for
> that? or asking security guys?
> 
> https://www.owasp.org/index.php/Insecure_Temporary_File
> > Finally, mkstemp() is a reasonably safe way to create temporary files.

In case you consider the implementation insecure, I think it's imperative to contact upstream and solve the issue with them. Fedora is no such place and in the end this needs to be resolved upstream anyway.

> Are you sure?
> It seems that the method "atomic_write_cache_file" which mktemp is called in
> is expecting as a attomic.
> But seeing the actual logic, I think the logic [2] is not related to the
> called method atomic_write_cache_file. As you know, Mutex itself is used for
> native thread. But it does not guarantee atomic access if lock is not used.
> It depends on the implementation.

Yes, like I wrote the mutex is used on Ruby level, not on C level. Whether that's secure or not is not for me to decide. However, there are two more conditions that'd have to be met for this to be a successfull attack, like I wrote already:

> Furthermore, reading CAPEC[3], as suggested by rpmlint, none of the Attack Prerequisites are not met AFAICT.
> [3] http://capec.mitre.org/data/definitions/29.html

________________

> For example it seems that when below logic is called several times at same
> timing, it does not guarantee the atomic access.
> 
> ```
> irb(main):001:0> require 'bootsnap'
> irb(main):006:0> Bootsnap.setup(cache_dir: '/tmp/foo', autoload_paths_cache:
> false)
> => :load_file
> ```

Disregarding the attack vector, for the sake of the argument -
  Actually, I think it should be safe. Because when you use native Ruby Threads (like in a rails app), Bootsnap runs `@mutex.synchronize { ... }` everywhere[*] and the requests should not collide.

[*] https://github.com/Shopify/bootsnap/blob/684acfd9b8c1298a026dd6b9c2ffeb173d11e949/lib/bootsnap/load_path_cache/cache.rb#L11

Comment 13 Pavel Valena 2018-06-11 12:15:05 UTC
Note: `bootsnap/setup` gets called only once, on the 'boot' in Rails.

https://github.com/rails/rails/blob/5-2-stable/railties/lib/rails/generators/rails/app/templates/config/boot.rb.tt

Comment 14 Vít Ondruch 2018-06-11 12:47:08 UTC
(In reply to Pavel Valena from comment #7)
> > I am fine to skip the test failure this time for armv7hl.
> 
> I'll file a bug when the component is created.
> In the meantime I'll exclude the armv7hl arch, as it does not work at all.

https://github.com/Shopify/bootsnap/issues/67
https://bugs.ruby-lang.org/issues/13670

Could you please check if there is fix in trunk or not?

Comment 15 Pavel Valena 2018-06-11 14:06:45 UTC
(In reply to Vít Ondruch from comment #14)
> (In reply to Pavel Valena from comment #7)
> > > I am fine to skip the test failure this time for armv7hl.
> > 
> > I'll file a bug when the component is created.
> > In the meantime I'll exclude the armv7hl arch, as it does not work at all.
> 
> https://github.com/Shopify/bootsnap/issues/67
> https://bugs.ruby-lang.org/issues/13670
> 
> Could you please check if there is fix in trunk or not?

Sure, thanks.

Comment 16 Vít Ondruch 2018-06-11 14:47:41 UTC
(In reply to Pavel Valena from comment #12)
> (In reply to Jun Aruga from comment #10)
> > How about using mkstemp instead of mktemp? Though I am not confident for
> > that? or asking security guys?
> > 
> > https://www.owasp.org/index.php/Insecure_Temporary_File
> > > Finally, mkstemp() is a reasonably safe way to create temporary files.
> 
> In case you consider the implementation insecure, I think it's imperative to
> contact upstream and solve the issue with them. Fedora is no such place and
> in the end this needs to be resolved upstream anyway.

If the mktemp is implemented as is described in the man(3) mktemp and the rails application is running in multiple processes, there can happen race condition IMO. In theory, this could be also exploited by TOCTOU. But the risk is rather low IMO, therefore I think it would be nice to raise this concern upstream, have link to the upstream issue somewhere in the .spec file and move on.

If this was be real concern, then every user of bootstrap would be vulnerable and  upstream needs to fix it anyway.

Comment 17 Jun Aruga 2018-06-11 15:05:36 UTC
Hi Pavel,
I do not order you anything.
I just reviewed your code following the review process.

And I was not sure the rpmlint's result of ERROR was affordable or not.
And if I was not confident for that, I can not accept your review.

Anyway, I asked the upstream project by myself.
https://github.com/Shopify/bootsnap/issues/174

I want to ask you to add the link somewhere in the spec file for the future.

Comment 18 Jun Aruga 2018-06-11 15:24:28 UTC
> But the risk is rather low IMO, therefore I think it would be nice to raise this concern upstream, have link to the upstream issue somewhere in the .spec file and move on.

Ah coincidence. Below is the link.

> Anyway, I asked the upstream project by myself.
> https://github.com/Shopify/bootsnap/issues/174

Comment 19 Pavel Valena 2018-06-13 08:35:25 UTC
I've added comments with links to issues above.

I was also able to isolate and backport a possible fix for the ARM issue:
  https://github.com/ruby/ruby/commit/c2007e191b2220619e524a8168411de7fdd2cae9
  (Ruby build - https://koji.fedoraproject.org/koji/taskinfo?taskID=27586882)

Now I'll try to test it further.

Scratch-build:
https://koji.fedoraproject.org/koji/taskinfo?taskID=27577493

Comment 20 Jun Aruga 2018-06-13 10:46:39 UTC
@Pavel,

Thank you for that.
I will accept the review for current spec file.
It's up to you to fix the ARM issue now or later.

Comment 21 Pavel Valena 2018-06-15 13:45:12 UTC
Correction - the commits most probably containing the fix are:
  https://github.com/ruby/ruby/commit/58a2084483ce8baaf90d7b1cb00e3fa9570fbc79
  https://github.com/ruby/ruby/commit/b6185e175cfac5bb8b56483c1b03997923af634e

But there seems to be no easy way to backport it (needs other commits / changes).
Therefore I'll build the bootsnap gem as is.

Thanks for the review!

Comment 22 Gwyn Ciesla 2018-06-15 14:04:52 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-bootsnap


Note You need to log in before you can comment on or make changes to this bug.