bufferoverflow/verdaccio-gitlab

Security concerns

Telokis opened this issue · 3 comments

Telokis commented

I am currently trying to implement #99 and noticed that anybody can access any package as long as they are authenticated through Gitlab. (https://github.com/bufferoverflow/verdaccio-gitlab/blob/v2.2.0/src/gitlab.js#L158)

From what I understand, to check whether the user is allowed to access a package or not, we only check if its user is set.

There is no verification made to ensure the user is accessing a scope he owns or that he has the proper access rights.

I am currently working on #99 so I could tackle this issue as well, I think. We just have to properly discuss the approach to solve it.

Here is what I planned to do to solve #99:

  • Add a configuration option allowAnyScope. This tells the plugin that accessing a non-owned scope might be allowed.
  • If allowAnyScope is true, we don't fail when packagePermit is false. Instead, we retrieve _package.publish and check if its value is within user.real_groups.
  • If the value is found, we allow the publishing.

Doing this was easy but I didn't expect to be able to see/access my published package. That's how I found the security concern.

Proposed solution:

I think the behavior for allow_access should follow the same pattern as allow_publish:
If the user doesn't have access to the group/project, it cannot access the packages within that scope.

Telokis commented

For the specific security concern I mentionned above, I choose this approach:

  • Both allow_access and allow_publish use a method allow_action with different arguments.
  • Added the special $owned-group permission that ensures the package is properly accessible by the user on Gitlab. This is the default behavior of allow_publish in the current code.
  • Added handling of $authenticated properly: it can explicitly be set and, if found, will simply ensure user.name !== undefined. This is the default behavior for access permission. (But I don't like that access and publish have different permissions, it may make more sense to let both be $owned-group but I didn't want to change the behavior too much).
  • Added handling of BUILTIN_ACCESS_LEVEL_ANONYMOUS for all actions. There is no reason a user shouldn't be able to config its Verdaccio so anyone can do anything for a specific action.
  • Added dynamic and complete error message mentionning the potential reasons why the access was refused.

The new code can be found here.

  • 👍1
Telokis commented

@bufferoverflow @dlouzan May I ask some feedback on this, please?

  • 👀1