[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 <a > href="https://shattered.io/";>Google</a>). 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@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
