Skip to content
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

Review Security and Privacy Considerations #1348

Closed
mmccool opened this issue Jan 10, 2022 · 12 comments
Closed

Review Security and Privacy Considerations #1348

mmccool opened this issue Jan 10, 2022 · 12 comments
Assignees
Labels
Propose closing Problem will be closed shortly if there is no veto. Security security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response.

Comments

@mmccool
Copy link
Contributor

mmccool commented Jan 10, 2022

The security and privacy considerations section needs to be reviewed and updated. In particular, there is a lot of discussion about ids being mutable, etc. which needs to be made consistent with discussion of ids elsewhere, and whether they refer to the Thing or the TD, how they are handled in Directories, handling of anonymous TDs, etc. There is also a problem with mutual authentication and local networks, and also no reference to the Security Best Practices document (which is not however, feasible until we actually publish a first draft!).

Deadline: by end of January. Anyone who has a thought on additional considerations should add a note here and ideally links to related issues. We also need to consider where each consideration goes; each WoT deliverable should have a similar issue to this:

@mmccool mmccool added Security security-needs-resolution Issue the security Group has raised and looks for a response on. security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. labels Jan 10, 2022
@w3cbot w3cbot removed the security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. label Jan 11, 2022
@mmccool
Copy link
Contributor Author

mmccool commented Jan 17, 2022

Considerations to add:

  • Use of TD metadata to scan for vulnerable devices. Security metadata is provided. This is not secret information, but may make it easier for an attacker to identify devices with relatively weak security. Discussion: this also allows for audits of security. Mitigation: TDs can be protected from unauthorized access using directories. Check: is stronger security than what is stated in the TD permitted, i.e. can a TD state that a Thing uses nosec but then requires OAuth using protocol negotiation? We could add an "undef" scheme to be clearer that security is negotiated using the protocol, but this would require a normative change. NOTE: currently, under section 8.1, there are assertions REQUIRING the security information in the TD to be accurate and complete, so it's not possible to omit security information; a TD which is not complete in this regard is considered invalid. So we would either have to soften these or add an exception for the (proposed) "undef" scheme. Note that "undef" however would be a form of "security through obscurity", and makes it harder to automate when we want to ADD extra security (e.g. forwarding a LAN thing through a proxy to the outside...).
  • Use of inappropriate combinations, e.g. passwd with no encryption (e.g. http vs https). Mitigation: TDs should be audited for where they are deployed and their acceptable threat level and warnings generated.

@mmccool
Copy link
Contributor Author

mmccool commented Jan 17, 2022

Another consideration:

  • Metadata embedded in ids. (Check, may be discussed in discovery too). Ids should not contain embedded metadata such as the type of device or the owner. In fact to avoid collisions they should be either cryptographically generated or provided during the onboarding process with a hub, etc. NOTE: some of the examples may still have IDs that violate this rule. Should check and replace with suitable UUID. Mitigation: use UUIDs for IDs.

@mmccool
Copy link
Contributor Author

mmccool commented Jan 31, 2022

"inappropriate combinations" -> "security validation" mitigation

@mmccool
Copy link
Contributor Author

mmccool commented Feb 16, 2022

Another Security Consideration: HTML markup in strings such as "title" and "description" can lead to an injection attack if these are embedded into HTML via template (e.g. for a dashboard) without filtering. For example, an attacker could embed a script into such a string that triggers upon, for example, a mouse-in or even just a page load event, and such a script can access other content on the same page (such as other devices) and potentially even trigger IoT actions or access other systems on the LAN (if, as is often the case, the LAN is considered a "trusted environment").

Mitigation: HTML markup should be filtered out of strings used in templates.

Ideally HTML markup in such strings should be forbidden by the spec and checked by the validator. However this would require a normative change to the TD specification, so we have to get it in before the CR transition.

One concern I have though is whether we depend upon HTML markup for internationalization, e.g. to set the initial rendering direction of strings in bidirectional scripts. If so, IMO the validator should only allow very specific forms of markup for these specific purposes.

@danielpeintner
Copy link
Contributor

Mitigation: HTML markup should be filtered out of strings used in templates.

Ideally HTML markup in such strings should be forbidden by the spec and checked by the validator. However this would require a normative change to the TD specification, so we have to get it in before the CR transition.

I don't think we should say anything about filtering HTML or JavaScript code. I also don't think this is the point here.
I think, what we should make clear is that the string should not be interpreted as HTML or JS (i.e. calling Javascripts eval() or so).

@mmccool
Copy link
Contributor Author

mmccool commented Feb 20, 2022

@danielpeintner to be clear, the problem is that strings like "title" and "description" in TDs will often be put into generated HTML by applications reading them, for example to create dashboards. This is often done using a template, and then HTML markup in the strings, including scripts (which can get embedded into HTML in various ways), will get interpreted in the context of the browser viewing the dashboard. This will allow malicious TD providers to hijack the browser session of the user viewing the dashboard.

Some possible mitigations of this are as follows:

  1. Developers generating a dashboard using strings drawn from TDs need to sanitize those strings and/or escape any HTML (e.g. by replacing special characters like angle brackets and quotes with ampersand-escapes).
  2. Not allowing HTML markup in strings in the first place, so the validator will reject TDs with HTML markup in strings.

Personally, both of these are potentially leaky: devs may not do 1, and not all implementations will validate TDs when they should. Anyway, for now I'm going to write up a Security Consideration giving 1 as the mitigation, but I would feel better if 2 was ALSO done, and I'll create an issue for it. The one concern I have about suggesting 2 is whether there is any legitimate reason to use HTML markup, e.g. to embed special characters using ampersand escapes or for internationalization (to set initial script direction). Hmm... actually 1 will break use of HTML markup for internationalization purposes too. Sigh.

@mmccool
Copy link
Contributor Author

mmccool commented Feb 21, 2022

After thinking about it, disallowing HTML in TD strings doesn't solve the problem since other forms of markup like SQL can still cause problems, and we can't strip them ALL. So I have written in the new security consideration in PR #1402 that it's the user's responsibility to sanitize HTML in strings, and noted that this may also break HTML used to set text direction for internationalization, etc. Oh well. Propose closing the above issue.

@danielpeintner
Copy link
Contributor

Question w.r.t. PR #1402: Does it make sense to describe it in a broader manner without focusing to much on HTML. There is JavaScript that may cause issues.
Note: there may be many more attacks with other techniques (e.g., Xsstc for CSS etc.)

A naive example from my side:

Data in strings should not be considered safe (trusted?) without being validated for malicious content (see XSS attacks for HTML, JavaScript, Xsstc for CSS, ...).

@mmccool
Copy link
Contributor Author

mmccool commented Feb 22, 2022

@danielpeintner yes, the problem is that there are many ways stuff embedded in strings can cause problems when used without sanitization in templates. However, it's not possible to know WHICH context a string might be used in that will cause a problem. Sure, HTML templates are going to be common, but CSS, SQL, and other injection attacks might also occur. So pre-sanitization will not catch all issues. The current draft in PR #1402 just says that USERS of strings need to sanitize them before using them. This (in the case of HTML) also breaks use of HTML for solving internationalization problems so I added a note about that.

Unfortunately, while working on this issue @sebastiankb pointed out some Security Considerations under IANA, some of which overlap with some also mentioned in the S&P sections, but they are not cross-referenced, and there are also assertions in the IANA section (which is normative) while the S&P sections are not. The string sanitization issue is a different one than the eval() problem, but would logically go under the IANA section if we were putting all data interpretation security considerations there...

Anyway, I created an issue about this, Issue #1405, with my proposed solution: consolidate the S&P considerations in the S&P sections, then just reference the appropriate ones from the IANA section. This has the side effect of making the S&P sections normative if we want to keep the assertions.

@plehegar plehegar added security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. and removed security-needs-resolution Issue the security Group has raised and looks for a response on. labels Mar 7, 2022
@w3cbot w3cbot added security-needs-resolution Issue the security Group has raised and looks for a response on. and removed security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. labels Mar 8, 2022
@mmccool mmccool removed the security-needs-resolution Issue the security Group has raised and looks for a response on. label May 16, 2022
@w3cbot w3cbot added the security-needs-resolution Issue the security Group has raised and looks for a response on. label May 16, 2022
@mmccool
Copy link
Contributor Author

mmccool commented May 30, 2022

well, I keep trying to remove the security-needs-resolution label (which I put on by accident) but w3cbot keeps putting it back. @plehegar can you take this off? So far I have not seen any actual security review input on this.

@mmccool
Copy link
Contributor Author

mmccool commented Jul 4, 2022

We are probably done with this for TD 1.1, so propose closing. That should also take care of the incorrect "security-needs-resolution" label which I can't seem to get rid of.

@mmccool mmccool added the Propose closing Problem will be closed shortly if there is no veto. label Jul 4, 2022
@sebastiankb sebastiankb removed the security-needs-resolution Issue the security Group has raised and looks for a response on. label Jul 6, 2022
@sebastiankb
Copy link
Contributor

ok, I will close this issue

@w3cbot w3cbot added the security-needs-resolution Issue the security Group has raised and looks for a response on. label Jul 7, 2022
@plehegar plehegar added security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. and removed security-needs-resolution Issue the security Group has raised and looks for a response on. labels Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Propose closing Problem will be closed shortly if there is no veto. Security security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants