Bug 1586295

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.

I guess as below files are used for development, those should be included in %files doc

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

(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

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
```

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.
```

You can move CODE_OF_CONDUCT.md to %files doc?

Other things look okay!

> 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!

> 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
```

Ref: How to attack is written here: https://en.wikipedia.org/wiki/Time_of_check_to_time_of_use

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.

> 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.

(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

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

(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?

(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.

(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.

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.

> 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

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

@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.

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!

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-bootsnap