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

In README generated by bundle gem, do not fill rubygems.org install commands with the gem name automatically #6093

Merged
merged 1 commit into from
Dec 23, 2022

Conversation

mensfeld
Copy link
Member

@mensfeld mensfeld commented Dec 1, 2022

This PR comes as part of our security work. With @simi, @indirect, and @hsbt an increase in brandjacking attacks where the brand's origin comes from scanning GH for names defined in the README.md but not in RubyGems.

It seems that often the default flow for people is to start their gems and upload them to GitHub without actually releasing them to RubyGems. In such cases or in cases where the author does not want to publish to RubyGems at all, the package name can be taken over and used for malicious activities, hoping someone will follow along with the recommendation from the README to install from RubyGems.

@welcome
Copy link

welcome bot commented Dec 1, 2022

Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack.

For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide

mensfeld added a commit to rubygems/rubygems.org that referenced this pull request Dec 1, 2022
In reference to this PR (rubygems/rubygems#6093) we need to protect the placeholder name from being brand jacked:

`update_with_your_gem_name_prior_to_release_to_rubygems_org`
$ gem install <%= config[:name] %>
$ gem install UPDATE_WITH_YOUR_GEM_NAME_PRIOR_TO_RELEASE_TO_RUBYGEMS_ORG

TODO: Replace the `UPDATE_WITH_YOUR_GEM_NAME_PRIOR_TO_RELEASE_TO_RUBYGEMS_ORG` with your gem name before releasing it to RubyGems.org or delete this section if you do not plan to release to RubyGems.org. Please do not do it earlier due to security reasons.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In next section TODO is at top of the section. Would it make sense to follow and do the same here (move TODO to top of this section)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this todo up as well as the one from the first section to align them


## Installation

TODO: Replace the `UPDATE_WITH_YOUR_GEM_NAME_PRIOR_TO_RELEASE_TO_RUBYGEMS_ORG` with your gem name before releasing it to RubyGems.org or delete this section if you do not plan to release to RubyGems.org. Please do not do it earlier due to security reasons.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The security concern is when changing the gem name, not when deleting the section. I guess it's obvious but the current wording feels ambiguous in that regard?

I also think we should link somewhere to try explain what the security concern is, or explain it ourselves. I believe users are more likely to follow advice they actually understand and I believe most users are not aware of this problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The security concern is when changing the gem name, not when deleting the section.

Not exactly. Even if you do not release someone can release it for you and brandjack. Which was happening.

I also think we should link somewhere to try explain what the security concern is, or explain it ourselves. I

Fair point. Do you want me to write a blog post for https://blog.rubygems.org/ about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, isn't the safe path the exact opposite to what we are recommending?

Replace the UPDATE_WITH_YOUR_GEM_NAME_PRIOR_TO_RELEASE_TO_RUBYGEMS_ORG with your gem name before releasing it to RubyGems.org

Shouldn't it be after rather than before, or maybe right after if we want to recommend doing it at that same-ish time?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right after is a much better wording. Updating...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly. Even if you do not release someone can release it for you and brandjack. Which was happening.

Right, I meant it regarding the trailing "Please do not do it earlier due to security reasons" sentence. In the case of deleting the section because of not planning to release to rubygems.org, the timing doesn't really matter, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a blog post would be nice, but I think @simi has some concerns about whether that would actually give this attack more visibility than we want? I'll let @simi chime in!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like after was replaced with before, but PRIOR_TO was left unchanged.

I've opened #6338 to update this.

I also don't know if a blog post ended up being written, but if so, perhaps we can link to it.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to clarify wording a bit!

bundler/lib/bundler/templates/newgem/README.md.tt Outdated Show resolved Hide resolved
This PR comes as part of our security work. With @simi, @indirect, and @hsbt  an increase in brandjacking attacks where the brand's origin comes from scanning GH for names defined in the README.md but not in RubyGems.

It seems that often the default flow for people is to start their gems and upload them to GitHub without actually releasing them to RubyGems. In such cases or in cases where the author does not want to publish to RubyGems at all, the package name can be taken over and used for malicious activities, hoping someone will follow along with the recommendation from the README to install from RubyGems.
@deivid-rodriguez deivid-rodriguez changed the title Do not fill the template with the gem name automatically In README generated by bundle gem, do not fill rubygems.org install commands with the gem name automatically Dec 23, 2022
@deivid-rodriguez deivid-rodriguez merged commit 00ce17e into rubygems:master Dec 23, 2022
@mensfeld mensfeld deleted the patch-1 branch December 24, 2022 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants