solid / security-considerations

https://solid.github.io/security-considerations/
2 stars 1 forks source link

initial WebID Document considerations #9

Closed elf-pavlik closed 1 month ago

elf-pavlik commented 1 month ago

partially addresses #3


preview | diff

VirginiaBalseiro commented 1 month ago

I agree with @csarven 's change suggestion highlighting performance reasons for apps to read only one document. Performance is one aspect to consider, but there are other issues with this approach that should be taken into account, such as UX and DX considerations. From a UX perspective, it's a challenge to clearly communicate to users which of their profiles are editable and which are not, along with what actions they can take for each type.

On the DX side, this approach needs more thought and clear guidelines and tools to help developers implement these considerations without adding significant complexity to their code. Implementors should consider the burden on the client when weighing the different options.

Furthermore, this PR should explore the option of the server rejecting modifications to oidcIssuer (or any sensitive information), which is a viable approach that would relieve the burden on the client.

I also agree that if the document is not modifiable by Solid protocol, this document is not applicable.

elf-pavlik commented 1 month ago

Furthermore, this PR should explore the option of the server rejecting modifications to oidcIssuer (or any sensitive information), which is a viable approach that would relieve the burden on the client.

A future dedicated PR can explore this hypothesis. This PR includes countermeasures already available today and used in at least one major Solid deployment. Sub-resource access control, as far as I know, is only available in Trinpod, and there is no open draft for it. The WebID Profile draft suggests this possibility, but I don't know of any existing implementations. I understand the comparison to containment triples in containers. Still, I would also notice that container descriptions are one of the solid permathreads, and there is a considerable degree of divergence between existing implementations. I would be cautious with using one controversial feature as a precedence for another feature.

On the DX side, this approach needs more thought and clear guidelines and tools to help developers implement these considerations without adding significant complexity to their code. Implementors should consider the burden on the client when weighing the different options.

I assume that we are talking about some of the scenarios documented in https://github.com/solid/webid-profile/blob/main/notes/use-cases.md

Which use cases don't fit a generic discovery mechanism, in lines of type indexes or SAI or future unified system? If everything boils down to client-to-client discovery, I believe we should find a way to delegate it with a single predicate from the WebID Document, preferably set in the account bootstrapping stage. Otherwise, we can discuss the issues based on concrete real-world use cases.


One more practical consideration: due to (ehm. unfortunate) 'slash semantics,' once a WebID Document is created in Solid Storage, it may be very hard to 'pull it out.' While this functionality is still underspecified, it is a safer approach not to create WebID Documents inside a Solid Storage.

elf-pavlik commented 1 month ago

I would like us to agree and merge this PR during the CG plenary next week.

If something being proposed would lead to non-conformance to Solid Protocol 0.11, we should focus on that. Otherwise, we should accept it as an available option and follow up with more pull requests, providing other options.

If something is being planned for future Solid releases, that would make the proposed countermeasure lead to non-conformance. We can add an inline issue and track that change to the protocol while it is under consideration.

TallTed commented 1 month ago

[@elf-pavlik] I would like us to agree and merge this PR during the CG plenary next week.

Merging things which are known to be wrong is a bad idea. Too often, it leads to those wrong things being kept due to things like inertia and beliefs that "it wouldn't have been merged if it was wrong".

There are a number of open change suggestions/requests here. They should optimally all be brought to consensus, or if consensus is deemed impossible turned into new issues for later resolution. Such issues will at least make plain that it was merged with known concerns, so future fixes will be more likely.

jeff-zucker commented 1 month ago

I agree with @csarven and @VirginiaBalseiro that performance and DX issues should be taken into account. The diagram below outlines the major pathways and what they require.

I do not support the sentence that says "There is no requirement that a WebID document ...". This may or may not be true in the future and is irrelevant. Just say that one possible counter-measure is to store the WebID document on something other than a Solid Resource Server.

I also very much agree with them that having a server protect vulnerable triples while allowing write access to other portions of the profile should be mentioned.

I agree with them that a Solid Specification can't apply directly to something that isn't a Solid server, however I disagree that we should say nothing on the subject. I believe that we should define a Solid WebID Profile as a document that lives on a Solid Resource server and is either directly de-referenced from the WebID URI or pointed to from the WebID Document with a solid:profileDocument (or foaf:primaryTopicOf) predicated pointing to a document on a Solid Resource Server. This is option #2 on the diagram. The current Solid WebID Profile draft spec is option #3 and is I believe, the wild west.
webid-use-case

@elf-pavlik I may have misrepresented SAI, please correct me.

elf-pavlik commented 1 month ago

I do not support the sentence that says "There is no requirement that a WebID document ...". This may or may not be true in the future and is irrelevant. Just say that one possible counter-measure is to store the WebID document on something other than a Solid Resource Server.

If the Solid Protocol requires hosting WebID in Solid Storage, then the suggested countermeasure would be invalid since it would lead to non-conformance.

The only hard requirement for the proposed countermeasures is that they can not lead to non-conformance.

I also very much agree with them that having a server protect vulnerable triples while allowing write access to other portions of the profile should be mentioned.

This is a distinct countermeasure; as of Solid Protocol 0.11, it is not specified, and as far as I know, no storage implementation supports it. Once we merge this PR, which defines the attack, please make a dedicated PR, and we can discuss all the details as well as any needed inline issues/notes in that dedicated PR.

I agree with them that a Solid Specification can't apply directly to something that isn't a Solid server, however I disagree that we should say nothing on the subject.

Solid Storage (aka Resource Server) is just one of many Product Classes defined by the Solid Protocol or being dependent on. One external dependency is WebID and WebID Document. Solid Storage plays a role in a prerequisite of the described attack and is not used for hosting WebID Document in the proposed countermeasure.


All the discovery approaches are out of scope for this PR; we should schedule STM to follow up on that. None of the approaches currently proposed and implemented require that the WebID Document be hosted in Solid Storage. If any proposal depends on it, we can even add an inline issue to that specific proposal.

jeff-zucker commented 1 month ago

All the discovery approaches are out of scope for this PR;

Then I guess you will not get my approval for this PR since I do not believe that issues of DX should be that last thing considered and I find it absurd to put the Solid Profile beyond the range of the Solid Protocol with no mention of how ito discover anything that is in the range of the Solid Protocol. As described in this PR it is entirely the prerogative of the IdP what goes in the WebID Document and where it points. That, to me, is contrary to the entire spirit of Solid. I believe the security warning about the WebID Document belongs in the WebID-Profile spec in a context which covers both the security concern and the discovery methods needed to work within the constraints imposed by those concerns.

jeff-zucker commented 1 month ago

One of the reasons it is important to include discovery along with this warning is that the warning will mandate changes by server implementers. So, is NSS first going to remove the profile from the RS and then at some later stage decide what discovery methods it should provision the new non-RS profile with? That is not workable - we will end up with a variety of non-RS WebID documents with no consistency in what is in them and no way other than manual user intervention to modify them.

Another aspect this PR fails to deal with is the issue of self-hosting. How will self-hosting work -? Will users need to run an IdP and an RS?

elf-pavlik commented 1 month ago

I believe the security warning about the WebID Document belongs in the WebID-Profile spec in a context which covers both the security concern and the discovery methods needed to work within the constraints imposed by those concerns.

The WebID-Profile draft is not part of Solid Protocol 0.11, and AFAIK, it is also not being proposed for the Linked Web Storage WG. However, Solid Protocol 0.11 depends on both Solid-OIDC and WebID drafts.

https://solidproject.org/TR/protocol#normative-references

And proposed countermeasure is conformant with Solid Protocol 0.11

Another aspect this PR fails to deal with is the issue of self-hosting. How will self-hosting work -? Will users need to run an IdP and an RS?

I'm working on an extension for CSS that enables a configuration option which will create WebID documents outside of the storage. For people self-hosting CSS it will just require to use that configuration instead of what currently is a default config.

One of the reasons it is important to include discovery along with this warning is that the warning will mandate changes by server implementers.

The countermeasure proposed in this PR doesn't mandate anything; it just informs and documents already broadly used practice. If NSS prefers another countermeasure, it can be submitted via dedicated PR. While it is not required to create that dedicated PR, I would be interested to know the implementation status of another countermeasure in NSS.

elf-pavlik commented 1 month ago

Security Considerations for the discovery mechanism will need a few dedicated iterations; I created a first issue and will followup with an initial PR

pchampin commented 1 month ago

After listening to the discussion about this topic in today's CG meeting, here are my 2¢

I suggest to replace the countermeasure section with an issue block, explaining that the group is still seeking consensus on the proposed countermeasure(s).

My feeling is that we are much closer to consensus on the first part, and that would allow us to make progress. Then we can discuss on how to replace the issue block with actual proposals, and focus on this side of the discussion.

jeff-zucker commented 1 month ago

After listening to the discussion about this topic in today's CG meeting, here are my 2¢

  • there seem to be more or less consensus on the described exploit prereqs and attack, but
  • there is no consensus on the proposed countermeasure.

I suggest to replace the countermeasure section with an issue block, explaining that the group is still seeking consensus on the proposed countermeasure(s).

My feeling is that we are much closer to consensus on the first part, and that would allow us to make progress. Then we can discuss on how to replace the issue block with actual proposals, and focus on this side of the discussion.

Thank you,, I think this is the way to go. As it stands now the document recommends what may be the best security policy but is definitely the worst interoperability possibility. There are other avenues to explore that would both protect the sensitive data and allow some Solid Apps to modify the profile. For example, a server provider could name a couple of supported apps e.g. Penny or SolidOS which could modify the profile but forbid all other apps. I would approve this PR if the counter-measure section is dropped and it is a left as a future challenge for the community to come up with solutions.

jeff-zucker commented 1 month ago

All the discovery approaches are out of scope for this PR;

If that is truly the case then the counter-measure section should be removed since it depends on a predetermined solution to the discovery issue. You can't both refuse to discuss it and put forth a one-sided take on it.

elf-pavlik commented 1 month ago

If that is truly the case then the counter-measure section should be removed since it depends on a predetermined solution to the discovery issue. You can't both refuse to discuss it and put forth a one-sided take on it.

In what way does the countermeasure depend on data discovery, like type indexes or SAI authorization agents with agent registrations?

jeff-zucker commented 1 month ago

In what way does the countermeasure depend on data discovery, like type indexes or SAI authorization agents with agent registrations?

Look at my diagram above. Your counter-measure (which I am not opposed to) makes choices. There are other possible choices and, even given the choice of that counter-measure, there are a number of unsolved discovery issues. If choices about discovery are out of scope, then do not favor one of those choices in your counter-measures.

elf-pavlik commented 1 month ago

The proposed countermeasure does not depend on any data discovery mechanism. It is applicable even with no data discovery mechanism in place.

On the other hand, some of the possible approaches in your diagram depend on something that is not guaranteed by the Solid Protocol. Specifically, the WebID Document being exposed via Solid Protocol. If you consider proposing that Solid Protocol add that guarantee, it could be linked as an open issue from the countermeasure proposed in this PR, which is entirely valid under Solid Protocol 0.11

I'm going to move the countermeasure into a separate PR so we can at least merge the attack. IMO, this will create the appearance that, after a decade, Solid Protocol has a fatal vulnerability with no available countermeasures, even though the countermeasure proposed in this PR is widely used and available to everyone who wants to start providing account bundles. Solid Protocol 0.11 does not specify data discovery mechanism; at least two proposals are on the table for future versions, both of which can work with the proposed countermeasure in place.

elf-pavlik commented 1 month ago

This may be more helpful. If Solid Protocol draft was to have WebID-Profile as normative reference (dependency), and WebID-Profile draft required that WebID Document is hosted in Solid Storage. The proposed countermeasure would indeed be invalid since it wouldn't conform to that potential future version of Solid Protocol draft.

As of Solid Protocol draft 0.11 (present), it does not depend on the WebID-Profile draft. Even more, the WebID-Profile draft could change that requirement before potentially becoming a dependency of the Solid Protocol draft.

Even more interesting is that the WebID-Profile draft is not being proposed as input to the Linked Web Storage WG. This makes it hard for me to imagine how the final Technical Recommendation produced by the WG would depend on the WebID-Profile draft.

Given all the above, I stand by my conclusion that the countermeasure proposed in this PR is fully valid under the current Solid Protocol 0.11, and I don't see a path where TR published by the WG would make it invalid. And even if it somehow did we can always adjust this draft here accordingly.

I see a problem with the general lack of a clear strategy on how Solid Protocol is supposed to be composed of all the smaller specifications. I share some thoughts specifically on WebID-Profile draft in the:

Again, all that is out of the scope of this PR.

We don't do anything agile-like, where we make small incremental changes, refactor whenever necessary, and sometimes revert old choices based on experience gained in the process. We also don't do waterfall, where we have all the acceptance criteria defined upfront. I don't know what we do, but it doesn't seem to work.

elf-pavlik commented 1 month ago

The last commit replaces the first documented countermeasure with an inline issue linking to this PR

TallTed commented 1 month ago

I am not OK with this being merged without addressing all the change requests.

elf-pavlik commented 1 month ago

Sorry @TallTed I added your suggestions in a separate commit https://github.com/solid/security-considerations/commit/2de5442dd82e9b0c8a6bff7fee3caf594ac19ea9