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

raw_enable in rest2html #981

Closed
Rtzq0 opened this Issue Jan 6, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@Rtzq0

Rtzq0 commented Jan 6, 2017

Is it not a large security issue to have raw_enabled = True in

'raw_enabled': True,
?

I ask because I've already found 1 platform that uses this library that permits the insertion of raw html by importing this module.

An upstream shift would probably improve the security of many downstream projects.

EDIT: I see this was done by @kakwa under 68557d2 . Maybe there is some rationale behind this.

@Rtzq0

This comment has been minimized.

Show comment
Hide comment
@Rtzq0

Rtzq0 Jan 11, 2017

Now that it's been disclosed... https://gitlab.com/gitlab-org/gitlab-ce/issues/26411 .

It doesn't look like .. raw:: html works on github.com at all, as per https://github.com/Rtzq0/exampleproj . Hopefully that's intentional.

The newly released dependents feature shows that this project has 708 downstream repos. It might be good to at the very least mention this issue in the README so that people can be aware and not use this gem unsafely.

Rtzq0 commented Jan 11, 2017

Now that it's been disclosed... https://gitlab.com/gitlab-org/gitlab-ce/issues/26411 .

It doesn't look like .. raw:: html works on github.com at all, as per https://github.com/Rtzq0/exampleproj . Hopefully that's intentional.

The newly released dependents feature shows that this project has 708 downstream repos. It might be good to at the very least mention this issue in the README so that people can be aware and not use this gem unsafely.

@Rtzq0

This comment has been minimized.

Show comment
Hide comment
@Rtzq0

Rtzq0 Jan 11, 2017

The writeup on the full attack that this choice enabled is live over at https://www.taos.com/2017/01/10/gitlab-markup-supply-chain-vulnerability/ .

Basically if a downstream app uses this to render ReST from the user displayed to other users, they have a vuln.

Rtzq0 commented Jan 11, 2017

The writeup on the full attack that this choice enabled is live over at https://www.taos.com/2017/01/10/gitlab-markup-supply-chain-vulnerability/ .

Basically if a downstream app uses this to render ReST from the user displayed to other users, they have a vuln.

@cnelsonsic

This comment has been minimized.

Show comment
Hide comment
@cnelsonsic

cnelsonsic commented Jan 11, 2017

See also #526

@Rtzq0

This comment has been minimized.

Show comment
Hide comment
@Rtzq0

Rtzq0 Jan 11, 2017

@cnelsonsic thanks for that. It is of course a decision by the devs if that fits their security model, but it looks like it's not actually what's live on github.com.

Additionally, if that is the desired outcome, maybe just a nice big warning sticker.

Rtzq0 commented Jan 11, 2017

@cnelsonsic thanks for that. It is of course a decision by the devs if that fits their security model, but it looks like it's not actually what's live on github.com.

Additionally, if that is the desired outcome, maybe just a nice big warning sticker.

@kivikakk

This comment has been minimized.

Show comment
Hide comment
@kivikakk

kivikakk Mar 20, 2017

Member

As a rule, markup doesn't do any sanitisation. Note that the Markdown rendering supplied by markup (via commonmarker) likewise allows raw HTML, so rest2html allowing raw HTML is consistent.

Per the README, after markup does its magic, github.com itself then aggressively sanitises the output HTML. I don't think we'll change markup's own operation here, as sanitisation needs vary depending on the end use.

Member

kivikakk commented Mar 20, 2017

As a rule, markup doesn't do any sanitisation. Note that the Markdown rendering supplied by markup (via commonmarker) likewise allows raw HTML, so rest2html allowing raw HTML is consistent.

Per the README, after markup does its magic, github.com itself then aggressively sanitises the output HTML. I don't think we'll change markup's own operation here, as sanitisation needs vary depending on the end use.

@kivikakk kivikakk closed this Mar 20, 2017

@Rtzq0

This comment has been minimized.

Show comment
Hide comment
@Rtzq0

Rtzq0 Mar 20, 2017

@kivikakk you could at least include a knob that defaults to 'off'. This has resulted in minimum 2 account compromises that I've located by this point. While I understand that needs vary depending on the end use, and can sympathize with your desire for compatibility and functionality the way in which you are calling docutils is cautioned against in the documentation for docutils:

http://docutils.sourceforge.net/docs/ref/rst/directives.html#raw-data-pass-through

If you absolutely cannot consider making this default to off, perhaps you might consider slightly more emphasis in the README on this point. There are 728 projects in your dependents listing, some of them quite popular. What's the harm in being slightly clearer in the explanation, for their sake?

Rtzq0 commented Mar 20, 2017

@kivikakk you could at least include a knob that defaults to 'off'. This has resulted in minimum 2 account compromises that I've located by this point. While I understand that needs vary depending on the end use, and can sympathize with your desire for compatibility and functionality the way in which you are calling docutils is cautioned against in the documentation for docutils:

http://docutils.sourceforge.net/docs/ref/rst/directives.html#raw-data-pass-through

If you absolutely cannot consider making this default to off, perhaps you might consider slightly more emphasis in the README on this point. There are 728 projects in your dependents listing, some of them quite popular. What's the harm in being slightly clearer in the explanation, for their sake?

@kivikakk

This comment has been minimized.

Show comment
Hide comment
@kivikakk

kivikakk Mar 20, 2017

Member

@Rtzq0 my concern with adding such a knob is the lack of consistency would be even more dangerous; we'd have to add it to all (non-rST) formats, and that's not even necessarily trivial. I'll improve the README in this respect.

Member

kivikakk commented Mar 20, 2017

@Rtzq0 my concern with adding such a knob is the lack of consistency would be even more dangerous; we'd have to add it to all (non-rST) formats, and that's not even necessarily trivial. I'll improve the README in this respect.

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