search

raku-community-modules / DBIish

Database interface for Raku
89 stars 31 forks source link

[SECURITY] Driver role exposes ALL connections made from EVERY place in the code. #100

Open bbkr opened 7 years ago

bbkr commented 7 years ago
  1. Connect to random database to get DBDish::mysql::Connection instance
  2. Call .parrent() on this instance to get DBDish::mysql driver
  3. Browse public %.Connections hash values to get other DBDish::mysql::Connection instances from outside current scope. Or even created on another threads.

I'm not sure what is the purpose of this hash in the first place. Why driver needs to remember which connections were made?

This is very serious security issue :(

moritz commented 7 years ago

This is not a security issue. Perl 6, like most other programming languages, does not offer secure isolation between program parts. You cannot base your security design on one part of the program not being able to access memory that another part in the same processes uses, simply because there are low-level APIs or even outright memory manipulation that can (potentially) give you access to any data in the process space.

Moreover, it stands to reason that if one part of a program can access credentials to build a DB connection, it stands to reason that another part of the same program can access the same credentials.

We can have a discussion whether the connection caching is a sensible feature, but not in the context of treating it as a security vulnerability.

bbkr commented 7 years ago

Thanks for explanation.

I think reasonable compromise would be to remove access to other connections on high API level - for example by making this hash private.

This would also fix bizarre connection introspection output (.gist) - for example my code does sometimes 100+ concurrent connections to different databases and by this public .parrent accessor gist produces 30 screens of output (because all Prepared statement objects in foreign connections are also visible).

moritz commented 7 years ago

If the bizarre .gist output is the problem, we can customize that.

I'm not married to this API, but so far I haven't heard any valid argument against it either. What problem are we trying to solve by removing this, if it's neither the apparent security issue nor the somewhat superficial gist issue?

Skarsnik commented 7 years ago

Hm, .parent and %.Connection probably should not be public outside DBIish (not sure how you can do this since we have no way to share ownership of an attribute/method to other class). Not sure what use could they have outside the module.

2017-09-21 9:40 GMT+02:00 Moritz Lenz notifications@github.com:

If the bizarre .gist output is the problem, we can customize that.

I'm not married to this API, but so far I haven't heard any valid argument against it either. What problem are we trying to solve by removing this, if it's neither the apparent security issue nor the somewhat superficial gist issue?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/perl6/DBIish/issues/100#issuecomment-331077898, or mute the thread https://github.com/notifications/unsubscribe-auth/AMYVf2OmzcJ4TcSbZ-lzrPSt6y2581IYks5skhL2gaJpZM4PenTj .

-- Sylvain "Skarsnik" Colinet

Victory was near but the power of the ring couldn't be undone

bbkr commented 7 years ago

Hm, .parent and %.Connection probably should not be public outside DBIish

Indeed. As @moritz explained we will never get complete memory isolation between 2 connections but at least foreign connections will not be visible directly through class public interface. I'd say this will be enough to close this issue.