Modify

Opened 8 years ago

Closed 8 years ago

#13040 closed defect (fixed)

Script injection

Reported by: Jun Omae Owned by: tkob-trac
Priority: normal Component: MermaidMacro
Severity: normal Keywords:
Cc: Trac Release:

Description

Any scripts can be injected via id argument:

{{{#!Mermaid id="''><script>alert(42)</script>"
....
}}}
  • mermaidmacro/1.0/tracmermaid/mermaid.py

    diff --git a/mermaidmacro/1.0/tracmermaid/mermaid.py b/mermaidmacro/1.0/tracmermaid/mermaid.py
    index 0f871da..e50b617 100644
    a b class MermaidMacro(WikiMacroBase): 
    3333        if args == None or 'id' not in args:
    3434            id_attr = ''
    3535        else:
    36             id_attr = 'id=%s' % args['id']
     36            id_attr = 'id=%s' % escape(args['id'])
    3737        url_escaped_content = urllib2.quote(content)
    3838        div = """<div class="mermaid"
    3939                      %s

Attachments (2)

0005-TH13040-Fix-possible-script-injection-vulnerability.patch (1.1 KB) - added by Christian Boos 8 years ago.
incremental patch, on top of 004 in #13042
0005-TH13040-Fix-possible-script-injection-vulnerability.2.patch (1.3 KB) - added by Christian Boos 8 years ago.
take 2, add missing "" after id=

Download all attachments as: .zip

Change History (7)

comment:1 Changed 8 years ago by Jun Omae

The same issue can be occurred via wiki page named with "><script>alert(42)</script>.

  • mermaidmacro/1.0/tracmermaid/mermaid.py

    diff --git a/mermaidmacro/1.0/tracmermaid/mermaid.py b/mermaidmacro/1.0/tracmermaid/mermaid.py
    index 0f871da..c491e46 100644
    a b class MermaidMacro(WikiMacroBase): 
    4343                      data-mermaidsource="%s">%s</div>"""
    4444        return div % (
    4545                id_attr,
    46                 formatter.context.resource.realm,
    47                 formatter.context.resource.id,
    48                 formatter.context.resource.version or '',
     46                escape(formatter.context.resource.realm),
     47                escape(formatter.context.resource.id),
     48                escape(formatter.context.resource.version or ''),
    4949                escape(url_escaped_content),
    5050                escape(content))
    5151

comment:2 Changed 8 years ago by Christian Boos

Oh, good catch, didn't see that one...

Just in case you're also testing my changes, I even have one more pending improvement, the support for TracLinks in flow charts.

I'll first version the changes locally so that we can have separated patches per ticket.

Changed 8 years ago by Christian Boos

incremental patch, on top of 004 in #13042

comment:3 Changed 8 years ago by Christian Boos

For good measure. Now you have all my changes. Would be nice if tkob had a github repo for this plugin, but it seems not.

Changed 8 years ago by Christian Boos

take 2, add missing "" after id=

comment:4 Changed 8 years ago by Christian Boos

By the way, it seems possible to insert <script> tags via the "normal" mermaid editor. Try the following:

sq["node <b>text</b><script>alert('bad!')</script>"] --> ci((Circle shape))

in http://knsv.github.io/mermaid/live_editor/.

However, via the MermaidMacro it seems OK, the alert doesn't show up, though the text is still bold. I haven't understood yet the difference between the two situations, i.e. the generated markup seems to be the same. Maybe you have a clue and could tell if there's a possible injection risk via this method?

Last edited 8 years ago by Christian Boos (previous) (diff)

comment:5 Changed 8 years ago by tkob-trac

Resolution: fixed
Status: newclosed

In 16179:

Add escape to avoid script injection, fixes #13040

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain tkob-trac.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.