security: templates can access c.SQL_ALCHEMY_URL

[binary1230]

We filter out the rest of the secrets, but for some reason (likely because it's a function), templates can still access the SecretConfig::sql_alchemy_url() function and print the DB user/pass in a template.

@jcooter figured this out

[EliAndrewC]

Templates can also access ANY of the stuff in SecretConfig, e.g. they can access c.STRIPE_SECRET_KEY and c.AWS_SECRET_KEY.

I don't consider this a problem at present, because templates are basically the same as code for us; we currently only render templates which we write and put in our repos. So at the present time, worrying about the fact that a template can access c.STRIPE_SECRET_KEY is like worrying about how a Python function can access c.STRIPE_SECRET_KEY.

That will all change if we ever start doing things like allowing users or admins to write templates which will be rendered into webpages or emails. For example, if we create a form that allows an admin to craft an automated email, then it's a much bigger deal. At that point we'd probably just not pass c to the user-provided templates in the first place, and instead pass a stripped-down version, or something.

[binary1230]

what was the point of the [secret] settings area of the INI file then? I thought we were filtering it from templates originally? might be remembering that wrong

[EliAndrewC]

We have a magfest.js Angular directive which defined a c Javascript object that we can use to get access to our config settings. You can see it at https://prime.uber.magfest.org/uber/static_views/magfest.js

Obviously we don't want any of our super-secret stuff like API keys and db passwords to end up there, which is why we created SecretConfig. Anything defined there is omitted from showing up in that file, and we can eventually exclude it from other places as well if/when we find ourselves needing to e.g. render a template with restricted privileges.

[earl7399]

Does this mean that the way things stand now, you are trusting every person who sets up a dev env not to abuse the templates, exactly like @nikgod did? I don't think this is a "Good Plan".

Robert Earl r.a.earl@gmail.com FAX: 1-440-919-5086

On Thu, Sep 24, 2015 at 11:03 AM, Eli Courtwright notifications@github.com wrote:

We have a magfest.js Angular directive which defined a c Javascript object that we can use to get access to our config settings. You can see it at https://prime.uber.magfest.org/uber/static_views/magfest.js

Obviously we don't want any of our super-secret stuff like API keys and db passwords to end up there, which is why we created SecretConfig. Anything defined there is omitted from showing up in that file, and we can eventually exclude it from other places as well if/when we find ourselves needing to e.g. render a template with restricted privileges.

— Reply to this email directly or view it on GitHub https://github.com/magfest/ubersystem/issues/1532#issuecomment-142955641 .

[kitsuta]

They'd be accessing junk data. Even when you pull from production config, you can't get any of the actually secret stuff, which lives solely on mcp.magfest.net.

On Thu, Sep 24, 2015 at 12:06 PM Robert Earl notifications@github.com wrote:

Does this mean that the way things stand now, you are trusting every person who sets up a dev env not to abuse the templates, exactly like @nikgod did? I don't think this is a "Good Plan".

Robert Earl r.a.earl@gmail.com FAX: 1-440-919-5086

On Thu, Sep 24, 2015 at 11:03 AM, Eli Courtwright < notifications@github.com> wrote:

We have a magfest.js Angular directive which defined a c Javascript object that we can use to get access to our config settings. You can see it at https://prime.uber.magfest.org/uber/static_views/magfest.js

Obviously we don't want any of our super-secret stuff like API keys and db passwords to end up there, which is why we created SecretConfig. Anything defined there is omitted from showing up in that file, and we can eventually exclude it from other places as well if/when we find ourselves needing to e.g. render a template with restricted privileges.

— Reply to this email directly or view it on GitHub < https://github.com/magfest/ubersystem/issues/1532#issuecomment-142955641> .

— Reply to this email directly or view it on GitHub https://github.com/magfest/ubersystem/issues/1532#issuecomment-142974459 .

[EliAndrewC]

Depends how you mean. Dev environments won't actually have values set for things like our secret keys and such, so people who set them up won't have access to those period. In theory someone could make a pull request which adds secret data to a template or which creates an Ajax handler which returns secret data to the user or which configures an automated email which emails our secret info in the subject line of an email to every attendee. The safeguard against all of those things happening is us not accepting the pull request.

[earl7399]

What exactly did @nikgod get then?

https://files.slack.com/files-pri/T02LNLJFA-F0B8F26HL/fail.png seems to show userid/pass for database access.

Robert Earl r.a.earl@gmail.com FAX: 1-440-919-5086

On Thu, Sep 24, 2015 at 12:12 PM, Eli Courtwright notifications@github.com wrote:

Depends how you mean. Dev environments won't actually have values set for things like our secret keys and such, so people who set them up won't have access to those period. In theory someone could make a pull request which adds secret data to a template or which creates an Ajax handler which returns secret data to the user or which configures an automated email which emails our secret info in the subject line of an email to every attendee. The safeguard against all of those things happening is us not accepting the pull request.

— Reply to this email directly or view it on GitHub https://github.com/magfest/ubersystem/issues/1532#issuecomment-142975764 .

[jcooter]

So here's the problem with allowing templates access to this stuff:

Say there's a hypothetical vulnerability that lets someone overwrite files on the filesystem. They decide that they're going to change the login page template (or an email template, or anything else that's shown to the user in question) to something that spits out everything in the object c.

Then, all they have to do is wait for the email, or navigate to the page. If it's something public like the login page, they don't even have to be logged in.

[kitsuta]

@earl7399 He got his personal copy of the database URL. Which he could also have gotten by looking straight in his config file.

If there's a vulnerability that lets people overwrite files, are templates really our main concern?

[jcooter]

Yes, one of them. I mean you can break a whole bunch of stuff by overwriting files, but assuming that you can't actually read the contents of the files (which is pretty common), the worst thing you can do is manipulate the templates to be able to exfiltrate data.

[EliAndrewC]

Say there's a hypothetical vulnerability that lets someone overwrite files on the filesystem.

The way our server currently works, this would allow them to overwrite our Python files, which would cause the webserver to automatically restart (even if this wasn't the case they could wait until the next deploy and their changes would be applied), which would allow them to execute arbitrary code. At that point, template rendering isn't really a concern, since they can do whatever they want.

I therefore don't see unrestricted template rendering as a vulnerability.

[binary1230]

@EliAndrewC agree pretty much with everything you're saying here.

it would be nice, but lower priority (since templates == code in terms of security), to have a c object that has blacklisted stuff like the angular JS.

could the secrets be easily excluded in in renderable_data() in decorators.py by changing this:

data['c'] = c

to the same thing the angular app is doing to not get the secrets?

data['c'] = {attr: getattr(c, attr, None) for attr in dir(c)}

(disclaimer: didn't test, and I suspect that the BEFORE_xxxx and AFTER_xxxx stuff would get broken).

Or perhaps have a wrapper class that overrides c.getattr() with a blacklist that disallows stuff that exists in _secret[] ?

[EliAndrewC]

You're correct that what we do for Angular would break for our templates because of stuff like BEFORE_ and AFTER_ and loads of other similar stuff. But there are definitely other similar options we can implement if/when we get to a point where it's necessary. What we do would depend on exactly what we needed, so I wouldn't want to propose a specific solution in advance of knowing what problem we're trying to address. But I can think of 3-4 ways off the top of my head that we could lock this down if we wanted to.