[devs: see penultimate paragraph (grep for "Thoughts")]
Jacek Materna wrote on Thu, May 11, 2017 at 14:56:03 +0200:
> For review.
Thanks for the patch. I'll review the form first, then the content.
Form:
1) Please use unified diff format, as generated by 'svn diff' or 'diff -u'.
What you posted was a reversed non-unified diff.
2) Please use a text/* MIME type. Naming the attachment with a .txt
extension usually does that. (It is easier to review patches that are
correctly MIME'd.)
3) One </a> closing tag was missing its slash.
4) Wrap the source to 80 columns. (I like breaking source lines at
fullstops, but that's my personal preference, not a house style.)
Content: (see inline)
> <div class="h3" id="shatterd-sha1">
> <h3>How do I protect my repository from the SHA-1 Shattered vulnerability?
> <a class="sectionlink" href="#shatterd-sha1"
> title="Link to this section">¶</a>
> </h3>
>
> <p>Subversion's use of SHA-1 in how it processes content is subject to hashing collisions as identified by Google). One of it's key assumptions in processing content is that SHA-1 is unique for all objects.</p>
The word "its" is misspelled, but more importantly, its referent is
unclear. I assume it means "Subversion's"?
> Subversion has two main areas of vulnerability.
> <br/>
> <ul>
> <li>The FS layer (repository) uses SHA-1.</li>
> <li>The Working Copy/RA layers use SHA-1.</li>
> </ul>
In user documentation I believe we say "FS backend" rather than "FS
layer".
> <p>
> The FS layer uses SHA-1 when identifying objects to store in the repository. To prevent non duplicate content from being stored that has identical SHA-1, upgrade to 1.9.6 (where would prevent storage of duplicates) or install the pre-commit hook found <a href="https://svn.apache.org/repos/asf/subversion/trunk/tools/hook-scripts/reject-known-sha1-collisions.sh">here<a>.
This may sound like nitpicking, but trunk is unstable code and not
covered by a release's legal umbrella. I think it would be better to
link to branches/1.10.x once it's created.
> As an aside, we welcome Windows developers to submit a pre-commit
> script for the Windows platform to the <a
> href="mailto:dev_at_subversion.apache.org">Developer List<a>.
Another option here is to link to the relevant section of HACKING, see
https://subversion.apache.org/patches. I don't have a preference one
way or the other.
Tangent: Speaking of that section of HACKING, I haven't read it in
a year or three. Perhaps it's time we reviewed it and ensured it was as
unscary and easy to follow as possible, to make it easier for people to
get involved.
> </p>
> <p>
> The working copy/RA layer uses SHA-1 for de-duplication of content stored in the working copy, and for performance reasons
> clients using the HTTP protocol will avoid fetching content with a SHA-1 checksum which has been fetched previously. There is no known workaround for this vector except to prevent storage of the colliding objects in the first place, via upgrade to 1.9.6 or installation of the aforementioned pre-commit script.
> </p>
> <p>
> Storing content with SHA1 collisions it not a supported use case. If you have repositories with colliding SHA-1 content we suggest you remove the content from you repository and upgrade to 1.9.6 to prevent future insertion.</p>
I don't think we recommend to *remove* the content outright but to gzip
it or otherwise transform it.
> </div>
The last few paragraphs of the patch basically summarize sha1-advisory.txt;
some of the text is from there too. I'm not sure how to best arrange
the information between the FAQ and the advisory (and, for that matter,
notes/api-errata/ if we choose to use it). On the one hand there's DRY,
on the other hand, there's room for summaries. Thoughts?
Thanks for the patch, Jacek. I like the content; we just need to decide
where it should live :)
Daniel
Received on 2017-05-14 02:21:15 CEST