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

Limited the usage of custom css to a specific div #349

Merged
merged 1 commit into from Mar 28, 2018

Conversation

Projects
None yet
3 participants
@mikicz
Copy link
Member

mikicz commented Mar 16, 2018

My thesis implementation includes limiting CSS from forks to only the content of the lesson itself, this is a preparation for that, so the diff is manageable.

@hroncok

This comment has been minimized.

Copy link
Member

hroncok commented Mar 16, 2018

Would applying the css attribute only to lesson-content automagically be too magical? I.e. not requiring the author of a lesson to know they need to do this?

@mikicz

This comment has been minimized.

Copy link
Member Author

mikicz commented Mar 16, 2018

I haven't thought of that... I don't think it would be too magical. I'm using cssutils to parse the CSS and to validate it, I'm guessing there's a way to change styles in the library. I'm gonna check tomorrow and come back to you on this.

@mikicz mikicz force-pushed the mikicz:page-css-restriction branch from d910a97 to 63213cd Mar 17, 2018

@mikicz

This comment has been minimized.

Copy link
Member Author

mikicz commented Mar 17, 2018

It was actually easier than I thought it would be. The extra requirement cssutils will be needed in the thesis implementation anyway.

parsed = cssutils.parseString(css)

for rule in parsed.cssRules:
rule.selectorText = ".lesson-content " + rule.selectorText

This comment has been minimized.

@hroncok

hroncok Mar 17, 2018

Member

Can you please add a docstring that explains what happens here?

This comment has been minimized.

@mikicz

mikicz Mar 17, 2018

Author Member

Sure :)

@mikicz mikicz force-pushed the mikicz:page-css-restriction branch from 63213cd to 32d23f5 Mar 17, 2018

@mikicz mikicz closed this Mar 17, 2018

@mikicz mikicz deleted the mikicz:page-css-restriction branch Mar 17, 2018

@mikicz mikicz restored the mikicz:page-css-restriction branch Mar 17, 2018

@mikicz mikicz reopened this Mar 17, 2018

@mikicz

This comment has been minimized.

Copy link
Member Author

mikicz commented Mar 17, 2018

Hm, GitHub could have asked me if I really want to delete the branch even though a PR with it is open. My bad, sorry for the confusing emails.

@hroncok I added the docstring as you requested...

"""
If the lesson defines extra css, the scope of the styles is limited to ``.lesson-content``,
a div which contains the actual lesson content. That way the styles can't disrupt
overall page appearance.

This comment has been minimized.

@encukou

encukou Mar 18, 2018

Member

That way the styles can't disrupt overall page appearance.

That is wrong. You can't rely on this function with untrusted input.
For example, this CSS would break pages:

    .dummy, * {
        position: absolute;
    }
@encukou

This comment has been minimized.

Copy link
Member

encukou commented Mar 18, 2018

Style note: please have the docstring say what the function does, e.g. Return lesson-specific extra CSS.

The rest of the docstring can give details.

@mikicz mikicz force-pushed the mikicz:page-css-restriction branch from 32d23f5 to 336c27d Mar 18, 2018

@mikicz

This comment has been minimized.

Copy link
Member Author

mikicz commented Mar 18, 2018

@encukou You're right of course. I changed it so each individual selector is altered, not just the first one.

@encukou

This comment has been minimized.

Copy link
Member

encukou commented Mar 19, 2018

It's nice to plug that particular hole, but can you be sure other such exploits don't exist? What if, say, CSS (and cssutils) add a parent selector some time in the future?

As it is, this PR makes it convenient to write correct CSS that doesn't break the rest of the page. But until proved otherwise, it's not secure against malicious input, and the dosctring shouldn't imply that.

@encukou

This comment has been minimized.

Copy link
Member

encukou commented Mar 19, 2018

(To be extra clear: at this point it's perfectly OK that this is not secure against malicious input. Just change the docstring.)

@mikicz

This comment has been minimized.

Copy link
Member Author

mikicz commented Mar 19, 2018

Fair, this PR is about the convenience, however, we will have to solve this problem at some point anyway since, if I understood correctly, securing against malicious input should be a part of the thesis implementation. If I should protect naucse against malicious CSS input, can we have the discussion here? If you think it's not important enough, says so, ignore the following text, I'll amend the docstring and that's going to be it.

cssutils is quite strict validation wise and if we freeze the requirement to 1.0 (so only CSS3 selectors are valid and no new CSS features are introduced), I think this code provides sufficient protection against bad input.

I might be wrong, of course, and tell me if I am, but I went over all the available CSS selectors and I believe this code covers all of them in regards of the style modifying .lesson-content or elements out of it.

Firstly, most of the selectors are ruled out by the space after .lesson-content, the space ensures all the pseudo-selectors and attribute-based selectors are just inside the div. The obvious culprits would be the + and ~ selectors. However, if somebody would like to use them to get out of .lesson-content, the lesson would have to define css like this:

+ div {
  ...
}

but such style wouldn't pass validation and a SyntaxErr would be raised.

A weird one I had to look up was :root, which wouldn't do anything even there wasn't an enforced space after .lesson-content since .lesson-content isn't used as the root element of naucse.

But again, if you don't think securing against malicious CSS input is important, even in the thesis implementation, says so, I'll shut up and just change the docstring.

@encukou

This comment has been minimized.

Copy link
Member

encukou commented Mar 19, 2018

OK, let's secure things! Talk to Miro regarding scope of the thesis.
A possible way out, both for initial implementation and for the thesis, would be to simply disable this feature for "external" content.

Your review of selectors looks comprehensive; I didn't check it yet though, and it looks like checking it will take some time. And I'm worried about bugs in cssutils.
If you freeze the cssutils requirement, put a comment about which code needs a new audit when the library is updated. (Another point to think about is that freezing a version might block future security updates. We're getting into deployment, rather than implementaition; that's probably out of scope.)

Or – more securely – do a whitelist. Allow only the minimum necessary.
For example, only allow defining simple classes in the extra CSS, and check that each such definition only contains simple property-value pairs, preferably with whitelisted property names and some checks for the values.

@hroncok

This comment has been minimized.

Copy link
Member

hroncok commented Mar 19, 2018

Or you could just change the docstring and figure this out in a follow-up PR.

@mikicz

This comment has been minimized.

Copy link
Member Author

mikicz commented Mar 19, 2018

Well, I guess it mostly depends on what you guys want. If you're both fine with disabling the feature for "external" content, I'd honestly go for that right now since it makes a lot of stuff easier. It even obsoletes this PR, which is really only a preparation for securing naucse against malicious CSS...

@hroncok

This comment has been minimized.

Copy link
Member

hroncok commented Mar 19, 2018

What does disabling mean here?

  1. There is content with custom css.
  2. I fork naučse, create my run with a small little change in content from 1.
  3. Do I loose custom css that I haven't touched?
@mikicz

This comment has been minimized.

Copy link
Member Author

mikicz commented Mar 19, 2018

Yeah, I understood it like that.
Or I could disable editing custom css (if the css provided by a fork was the same as the css of the same lesson in the root repository, it would be used, otherwise the css would be ignored)? But that seems to me like bending over backwards tbh...

@encukou

This comment has been minimized.

Copy link
Member

encukou commented Mar 20, 2018

I would be fine with that as a known limitation of the initial implementation.

@mikicz

This comment has been minimized.

Copy link
Member Author

mikicz commented Mar 23, 2018

On Wednesday we agreed not to disable this functionality in the forks and to use the code from this PR in the thesis implementation, so I added a comment to requirements why the library version is frozen and expanded a bit about the safety in the docstring of the method.

@encukou

This comment has been minimized.

Copy link
Member

encukou commented Mar 27, 2018

@mikicz Please look at this: https://github.com/maxchehab/CSS-Keylogging
While the specific attack doesn't apply to the current naucse, the fact that such creative use of CSS is possible did make me change my mind a bit. More specifically, revert to where I was before this discussion.

The security of your approach relies on two assumptions:

  • The CSS can't "escape" .lesson-content
  • It's "just" CSS, it can't do much damage

The second is right out, and I don't trust the first – it's really just a matter of time before someone finds a creative loophole in either CSS itself or the library we're using.

My mind is set now: before I allow CSS from untrusted users, there needs to be a strict whitelist.

BUT, the contribution you're making with your thesis does not allow content from untrusted users – repo owners still need to add courses to the list, which implies some trust & accountability, and as long as naucse is limited to our community, I'm perfectly fine with just trusting the course authors.
So, again, you're OK to ignore this problem for the thesis implementation – it's only tangentially related to the thesis topic. (You should mention it in the thesis, though – say you thought about it, but solving it fully is out of scope.).

Freezing cssutils~=1.0 is unnecessary – it doesn't provide adequate security anyway.
I don't fully agree with the latest docstring addition – it looks like a security audit of cssutils has been done, and I'm sorry to say I don't trust you to do that properly. I wouldn't trust myself either, if that's any consolation.
Please just replace it with a simple This doesn't protect against malicious input. and let's get this PR merged. And open a new issue/PR for more discussion.

@mikicz mikicz force-pushed the mikicz:page-css-restriction branch from c986a4f to af6f33c Mar 27, 2018

@mikicz

This comment has been minimized.

Copy link
Member Author

mikicz commented Mar 27, 2018

Thanks for the thorough review, you're absolutely right of course and I apologize for my arrogance in pushing this as a "safe" option.

I amended the docstring and updated the requirements.

@encukou

This comment has been minimized.

Copy link
Member

encukou commented Mar 28, 2018

Thanks. Sorry for killing the fun :(

@encukou encukou merged commit af6f33c into pyvec:master Mar 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mikicz mikicz deleted the mikicz:page-css-restriction branch Apr 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment