<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 10, 2019 at 11:00 AM Ivan Pozdeev via Python-Dev <<a href="mailto:python-dev@python.org">python-dev@python.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
  
    
  
  <div bgcolor="#FFFFFF">
    <p><br>
    </p>
    <div class="gmail-m_7935463884679230967moz-cite-prefix">On 10.04.2019 7:30, Karthikeyan wrote:<br>
    </div>
    <blockquote type="cite">
      
      <div dir="ltr">
        <div dir="auto">Thanks Gregory. I think it's a good tradeoff to
          ensure this validation only for URLs of http scheme. 
          <div dir="auto"><br>
          </div>
          <div dir="auto">I also agree handling newline is little
            problematic over the years and the discussion over the level
            at which validation should occur also prolongs some of the
            patches. <a href="https://bugs.python.org/issue35906" target="_blank">https://bugs.python.org/issue35906</a>
            is another similar case where splitlines is used but it's
            better to raise an error and the proposed fix could be used
            there too. Victor seemed to wrote a similar PR like linked
            one for other urllib functions only to fix similar attack in
            ftplib to reject newlines that was eventually fixed only in
            ftplib</div>
          <div dir="auto"><br>
          </div>
          <div dir="auto">* <a href="https://bugs.python.org/issue30713" target="_blank">https://bugs.python.org/issue30713</a> </div>
          <div dir="auto">* <a href="https://bugs.python.org/issue29606" target="_blank">https://bugs.python.org/issue29606</a></div>
          <div dir="auto"><br>
          </div>
          <div dir="auto">Search also brings multiple issues with one
            duplicate over another that makes these attacks scattered
            over the tracker and some edge case missing. Slightly off
            topic, the last time I reported a cookie related issue where
            the policy can be overriden by third party library I was
            asked to fix it in stdlib itself since adding fixes to
            libraries causes maintenance burden to downstream libraries
            to keep up upstream. With urllib being a heavily used module
            across ecosystem it's good to have a fix landing in stdlib
            that secures downstream libraries encouraging users to
            upgrade Python too.<br>
            <br>
            <div class="gmail_quote" dir="auto">
              <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> </blockquote>
            </div>
          </div>
        </div>
      </div>
    </blockquote>
    <p>Validation should occur whenever user data crosses a trust
      boundary -- i.e. when the library starts to assume that an
      extracted chunk now contains something valid.</p>
    <p><a class="gmail-m_7935463884679230967moz-txt-link-freetext" href="https://tools.ietf.org/html/rfc3986" target="_blank">https://tools.ietf.org/html/rfc3986</a>
      defines valid syntax (incl. valid characters) for every part of a
      URL -- _of any scheme_ (FYI, \r\n are invalid everywhere and the
      test code for     `data:' that Karthikeyan referred to is raw data
      to compare to rather than a part of a URL). It also obsoletes all
      the RFCs that the current code is written against.</p>
    <p>AFAICS, urllib.split*
      fns (obsoleted as public in 3.8) are used by both urllib and
      urllib2 to parse URLs. They can be made to each validate the chunk
      that they split off. urlparse can validate the entire URL
      altogether.<br>
    </p>
    <p>Also, all modules ought to use the same code (urlparse looks like
      the best candidate) to parse URLs -- this will minimize the attack
      surface.<br>
    </p>
    <p>I think I can look into this later this week.</p></div></blockquote><div>My PR as of last night cites that RFC and does validation in http.client while constructing the protocol request payload.  Doing it within split functions was an initial hack that looked like it might work but didn't feel right as that isn't what people expect of those functions and that turned out to be the case as I tested things due to our mess of codepaths for opening URLs, but they all end with http.client so yay!</div><div><br></div><div>I did <i>not</i> look at any of the async http client code paths.  (legacy asyncore or new asyncio).  If there is an issue there, those deserve to have their own bugs filed.</div><div><br></div><div>As for third party PyPI libraries such as urllib3, they are on their own to fix bugs.  If they happen to use a code path that a stdlib fix helps, good for them, but honestly they are much better off making and shipping their own update to avoid the bug.  Users can get it much sooner as it's a mere pip install -U away rather than a python runtime upgrade.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF"><p>Fixing this is going to break code that relies on the current
      code accepting invalid URLs. But the docs have never said that
      e.g. in urlopen, anything apart from a (valid) URL is accepted (in
      particular, this implies that the user is responsible for escaping
      stuff properly before passing it). So I would say that we are
      within our right here and whoever is relying on those quirks is
      and has always been on unsupported territory. <br></p></div></blockquote><div>yep.  even http.client.HTTPConnection.request names the function parameter "url" so anyone embedding whitespace newlines and http protocol strings within that is well outside of supported territory (as one example in our own test_xmlrpc was taking advantage of to test a malformed request). <br></div><div><br></div><div>I suggest following up on <a href="https://bugs.python.org/issue30458">https://bugs.python.org/issue30458</a> rather than in this thread.  the thread did its job, it directed our eyeballs at the problems. :)</div><div><br></div><div>-gps</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF"><p>
      Determining which of those quirks are exploitable and which are
      not to fix just the former is an incomparably larger, more
      error-prone and avoidable work. If anything, the history of the
      issue referenced to by previous posters clearly shows that this is
      too much to ask from the Python team.<br>
    </p>
    <p>I also see other undocumented behavior like accepting
      '<URL:<url>>' (also obsoleted as public in 3.8) which
      I would like to but it's of no harm.<br>
      <br>
    </p>
    <p>-- </p>
    <pre class="gmail-m_7935463884679230967moz-signature" cols="140">Regards,
Ivan</pre>
  </div>

_______________________________________________<br>
Python-Dev mailing list<br>
<a href="mailto:Python-Dev@python.org" target="_blank">Python-Dev@python.org</a><br>
<a href="https://mail.python.org/mailman/listinfo/python-dev" rel="noreferrer" target="_blank">https://mail.python.org/mailman/listinfo/python-dev</a><br>
Unsubscribe: <a href="https://mail.python.org/mailman/options/python-dev/greg%40krypto.org" rel="noreferrer" target="_blank">https://mail.python.org/mailman/options/python-dev/greg%40krypto.org</a><br>
</blockquote></div></div>