Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: neutral (nike.apache.org: local policy)
Message-ID: <50350846.5020900@aldan.algebra.com>
Date: Wed, 22 Aug 2012 12:26:46 -0400
From: "Mikhail T." <mi+thun@aldan.algebra.com>
User-Agent: Mozilla/5.0 (X11; FreeBSD i386;
 rv:14.0) Gecko/20120808 Thunderbird/14.0
MIME-Version: 1.0
To: dev@httpd.apache.org
CC: Nick Kew <nick@webthing.com>
Subject: Fixing apr_dbd_freetds (Re: Limitations of mod_dbd - single server
 per vhost)
References: <5033E113.90009@aldan.algebra.com>
 <2245AD18-14C9-4F86-A652-E92856B5D107@webthing.com>
 <A2FA726D-D2FF-4FA3-B382-F893207038F3@sharp.fm>
 <56F54E9D-2100-4A79-ABBE-0B45F0249FCE@webthing.com>
 <5034DCCC.7020104@aldan.algebra.com> <20120822154713.12598410@baldur>
In-Reply-To: <20120822154713.12598410@baldur>
Content-Type: multipart/alternative;
 boundary="------------030503020300090103050208"

This is a multi-part message in MIME format.
--------------030503020300090103050208
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Perhaps, this discussion should be happening on theticket  <https://issues.apache.org/bugzilla/show_bug.cgi?id=53666>  itself? Oh, well :) Comments inline:

On 22.08.2012 10:47, Nick Kew wrote:
> I think I've pointed a few people at the ODBC driver as an alternative.
> Do you have a strong reason to use FreeTDS rather than ODBC?
Yes, ODBC is yet another layer of abstraction. I'd understand, if apr_dbd.c used 
purely ODBC, leaving the ODBC implementation to deal with the backends' native 
drivers. But if apr_dbd offers "native" drivers of its own, better to use those, 
if possible. That's my "strong" reason :-)
> One question here may be, does anyone have the platforms to test-drive
> your patch?
I would not know... But, in my opinion, even without getting tested by anyone 
else, my patch is still a vast improvement over the current situation...
> It seems to do rather more than just fix a simple bug:
That's because the "bug" was not simple -- the driver remained broken ever since 
the apr_dbd.c was changed to do the parsing of query-templates centrally instead 
of delegating the job to each driver. Other drivers got updated back then, but 
not the FreeTDS one :-( It still tried to do its own parsing, and was failing...
> you've removed the untainting code which was part of protecting against
> injection attacks.
I don't think, the untainting was ever properly implemented (you can't do it 
right without prepared statements) and I don't believe, it is the driver's job 
to do it anyway. None of the other drivers do it either -- though they rely on 
the respective client-libraries. Sybase/FreeTDS client does not offer such 
checks -- so be it... The untainting was costly (done for each call) and not 
guaranteed -- better to leave it to the caller, IMHO.
> Have you implemented prepared statements properly for all backends?
To the best of my knowledge, neither the Sybase's db-client library nor the 
FreeTDS reimplementation of it offer prepared statements 
<http://lists.ibiblio.org/pipermail/freetds/2010q1/025493.html>, unfortunately. 
The current implementation certainly does not use them. Sybase offers a newer 
client interface (ct-lib), that does have prepared statements (see ct_dynamic() 
<http://infocenter.sybase.com/help/index.jsp?topic=/com.sybase.help.sdk_12.5.1.ctref/html/ctref/CHDIGFHG.htm>), 
but FreeTDS does not provide it, so, for the driver to be compatible with both, 
it has to use the old db-interface... To me this seems like an acceptable 
caveat, as long as it is known and documented.

My patch does not touch any of the other backends.

Perhaps, some day I (or someone else) will implement apr_dbd_sybase.c -- using 
the ct-interface, which would give prepared statements and other improvements. 
But that would have to be maintained outside of Apache, because, foolishly, 
Sybase would not open-source the client... It would also be Sybase-only (no MS 
SQL Server).

For the time being, my patch offers a major improvement over the status quo -- 
the driver becomes usable. I am, in fact, preparing to use it with RewriteMaps 
to do SEO for a giant site, that still uses an old Sybase-backed CMS. The 
RewriteRules will ensure, no tainted keys are passed to the queries and the 
DB-user's credentials will be limited to only SELECTs and only for a particular 
table to mitigate the risk of any SQL-injection attack.

Yours,

    -mi


--------------030503020300090103050208
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">
      <pre wrap="">Perhaps, this discussion should be happening on the <a href="https://issues.apache.org/bugzilla/show_bug.cgi?id=53666">ticket</a> itself? Oh, well :) Comments inline:
<a class="moz-txt-link-rfc2396E" href="https://issues.apache.org/bugzilla/show_bug.cgi?id=53666"></a></pre>
      On 22.08.2012 10:47, Nick Kew wrote:<br>
      <blockquote type="cite">
        <pre wrap="">I think I've pointed a few people at the ODBC driver as an alternative.
Do you have a strong reason to use FreeTDS rather than ODBC?</pre>
      </blockquote>
      <div align="justify">Yes, ODBC is yet another layer of
        abstraction. I'd understand, if <tt>apr_dbd.c</tt> used purely
        ODBC, leaving the ODBC implementation to deal with the backends'
        native drivers. But if apr_dbd offers "native" drivers of its
        own, better to use those, if possible. That's my "strong" reason
        :-)<br>
      </div>
    </div>
    <blockquote cite="mid:20120822154713.12598410@baldur" type="cite">
      <pre wrap="">One question here may be, does anyone have the platforms to test-drive
your patch?</pre>
    </blockquote>
    <div align="justify">I would not know... But, in my opinion, even
      without getting tested by anyone else, my patch is still a vast
      improvement over the current situation...<br>
    </div>
    <blockquote cite="mid:20120822154713.12598410@baldur" type="cite">
      <pre wrap="">It seems to do rather more than just fix a simple bug:</pre>
    </blockquote>
    <div align="justify">That's because the "bug" was not simple -- the
      driver remained broken ever since the <tt>apr_dbd.c</tt> was
      changed to do the parsing of query-templates centrally instead of
      delegating the job to each driver. Other drivers got updated back
      then, but not the FreeTDS one :-( It still tried to do its own
      parsing, and was failing...<br>
    </div>
    <blockquote cite="mid:20120822154713.12598410@baldur" type="cite">
      <pre wrap="">you've removed the untainting code which was part of protecting against
injection attacks.</pre>
    </blockquote>
    <div align="justify">I don't think, the untainting was ever properly
      implemented (you can't do it right without prepared statements)
      and I don't believe, it is the driver's job to do it anyway. None
      of the other drivers do it either -- though they rely on the
      respective client-libraries. Sybase/FreeTDS client does not offer
      such checks -- so be it... The untainting was costly (done for
      each call) and not guaranteed -- better to leave it to the caller,
      IMHO.<br>
    </div>
    <blockquote cite="mid:20120822154713.12598410@baldur" type="cite">
      <pre wrap="">Have you implemented prepared statements properly for all backends?</pre>
    </blockquote>
    <div align="justify">To the best of my knowledge, neither the
      Sybase's db-client library nor the FreeTDS reimplementation of it
      offer <a
        href="http://lists.ibiblio.org/pipermail/freetds/2010q1/025493.html">prepared
        statements</a>, unfortunately. The current implementation
      certainly does not use them. Sybase offers a newer client
      interface (ct-lib), that does have prepared statements (see <tt><a
href="http://infocenter.sybase.com/help/index.jsp?topic=/com.sybase.help.sdk_12.5.1.ctref/html/ctref/CHDIGFHG.htm">ct_dynamic()</a></tt>),
      but FreeTDS does not provide it, so, for the driver to be
      compatible with both, it has to use the old db-interface... To me
      this seems like an acceptable caveat, as long as it is known and
      documented.<br>
      <br>
      My patch does not touch any of the other backends.<br>
      <br>
      Perhaps, some day I (or someone else) will implement <tt>apr_dbd_sybase.c</tt>
      -- using the ct-interface, which would give prepared statements
      and other improvements. But that would have to be maintained
      outside of Apache, because, foolishly, Sybase would not
      open-source the client... It would also be Sybase-only (no MS SQL
      Server).<br>
      <br>
      For the time being, my patch offers a major improvement over the
      status quo -- the driver becomes usable. I am, in fact, preparing
      to use it with <tt>RewriteMap</tt>s to do SEO for a giant site,
      that still uses an old Sybase-backed CMS. The <tt>RewriteRule</tt>s
      will ensure, no tainted keys are passed to the queries and the
      DB-user's credentials will be limited to only SELECTs and only for
      a particular table to mitigate the risk of any SQL-injection
      attack.<br>
    </div>
    <br>
    Yours,<br>
    <blockquote>-mi<br>
    </blockquote>
  </body>
</html>

--------------030503020300090103050208--