[aur-dev] [PATCH] validate email and fully check existence of it
Lukas Fleischer
archlinux at cryptocrack.de
Tue Mar 20 15:33:32 EDT 2012
On Tue, Mar 20, 2012 at 12:06:03PM +0100, Ike Devolder wrote:
> On Tue, Mar 20, 2012 at 11:31:32AM +0100, Lukas Fleischer wrote:
> > On Tue, Mar 20, 2012 at 10:36:11AM +0100, BlackEagle wrote:
> > > - check if the format is valid
> > > - go and connect to the smtp server of the given domain and verify if
> > > the given email exists there
> >
> > I'm still not convinced that this is the right way to go. This check is
> > quite technical and looks error-prone, while it still allows anyone to
> > use arbitrary mail addresses (e.g. "archlinux at cryptocrack.de" or random
> > mail addresses from an email address list created by a crawler).
> >
> > As I said on IRC, there's probably only two (proper) ways to fix this:
> >
> > * Send verification mails.
>
> is the best solution, but this patch could be an intermediate until the
> verification stuff is in place
To be honest, I would prefer the first version (the one that checks
MX/A/AAAA records only and doesn't replace the filter_var() check) of
your patch as an intermediate. This is "good enough" for now - at least
until someone steps up and implements email verification. You probably
noticed yourself that dealing with all SMTP corner cases is tricky,
error-prone and a pain to maintain; and, as I said before, this can be
bypassed just as easy as the MX record check.
>
> > * Use captchas.
>
> this is only annoying the user, and rendered useless when using
> verification mail.
>
> >
> > I'll still add some comments to your patch below...
> >
> > > ---
> > > web/lib/aur.inc.php | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > 1 files changed, 74 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php
> > > index c662b80..3fc0a14 100644
> > > --- a/web/lib/aur.inc.php
> > > +++ b/web/lib/aur.inc.php
> > > @@ -80,7 +80,80 @@ function check_sid($dbh=NULL) {
> > > # verify that an email address looks like it is legitimate
> > > #
> > > function valid_email($addy) {
> > > - return (filter_var($addy, FILTER_VALIDATE_EMAIL) !== false);
> > > + // check against RFC 3696
> >
> > Please use tabs, not spaces, for indentation.
>
> sorry was on another configuration since i have to use spaces there
>
> >
> > > + if(filter_var($addy, FILTER_VALIDATE_EMAIL) === false) {
> > > + return false;
> > > + }
> > > +
> > > + // check dns for mx, a, aaaa records
> > > + list($local, $domain) = explode('@', $addy);
> > > + if(! (checkdnsrr($domain, 'MX') || checkdnsrr($domain, 'A') || checkdnsrr($domain, 'AAAA'))) {
> > > + return false;
> > > + }
> >
> > This check could be dropped if we actually decided for this complicated
> > method. We check for MX records below.
> >
> > > +
> > > + // get mx records and check full email address
> > > + $mxlist = array();
> > > + $mxweight = array();
> > > + getmxrr($domain, $mxlist, $mxweight);
> > > + $mx = array_combine($mxweight, $mxlist);
> > > + ksort($mx);
> >
> > You checked for A and AAAA records above but don't add them to the "$mx"
> > array here?
> >
> > > +
> > > + //smtp_test_email($addy, current($mx));
> >
> > Please strip commented code from patches.
>
> sorry
>
> >
> > > + foreach($mx as $prio => $mxsrv) {
> > > + if(smtp_test_email($addy, $mxsrv) === true) {
> > > + return true;
> > > + }
> > > + }
> > > +
> > > + return false;
> > > +}
> > > +
> > > +# verify that an email address exists on the smtp server
> > > +#
> > > +function smtp_test_email($addy, $mxsrv) {
> > > + if(($smtp = fsockopen($mxsrv, 25)) === false) {
> > > + return false;
> > > + }
> > > +
> > > + if(intval(preg_replace('/^\([0-9]{3}\).*/', '\1', fgets($smtp))) !== 220) {
> >
> > Better use substr() here:
> >
> > if (intval(substr(fgets($smtp), 0, 3)) !== 220) {
> > [...]
> >
> > Also, what about other error codes, such as 421?
>
> i think we can consider 421 as an error since we want to verify the
> complete email address
>
> >
> > > + smtp_close($smtp);
> > > + return false;
> > > + }
> > > +
> > > + fwrite($smtp, "HELO $mxsrv\r\n");
> > > + if(intval(preg_replace('/^\([0-9]{3}\).*/', '\1', fgets($smtp))) !== 250) {
> >
> > Same here.
> >
> > > + smtp_close($smtp);
> > > + return false;
> > > + }
> > > +
> > > + fwrite($smtp, "MAIL FROM: <mailtest at archlinux.org>\r\n");
> > > + if(intval(preg_replace('/^\([0-9]{3}\).*/', '\1', fgets($smtp))) !== 250) {
> >
> > Same here.
> >
> > > + smtp_close($smtp);
> > > + return false;
> > > + }
> > > +
> > > + fwrite($smtp, "RCPT TO: <$addy>\r\n");
> > > + $code = intval(preg_replace('/^\([0-9]{3}\).*/', '\1', fgets($smtp)));
> >
> > Same here.
> >
> > > + /**
> > > + * 250 = success
> > > + * 451 or 452 = address got greylisted but another error occured
> > > + * so assume ok
> > > + */
> > > + if($code !== 250 && $code !== 451 && $code !== 452) {
> >
> > What about 251? What about 450?
>
> 251 should be taken in
> then 551 comes in the picture and both would require this function to be
> run with the returned <forward-path>
> when we follow the forward-path stuff then we could run out of time if
> the following ones are also forwarding
>
> 450 is considered an error so i would let it fail there
>
> >
> > This is what I don't like about this patch... There's a dozens of corner
> > cases to consider and I don't think this is the right place to check for
> > these.
> >
> > > + smtp_close($smtp);
> > > + return false;
> > > + }
> > > +
> > > + smtp_close($smtp);
> > > + return true;
> > > +}
> > > +
> > > +# close smtp conneciton
> > > +#
> > > +function smtp_close(&$smtp) {
> > > + fwrite($smtp, "RSET\r\n");
> > > + fwrite($smtp, "QUIT\r\n");
> > > + fclose($smtp);
> > > }
> > >
> > > # a new seed value for mt_srand()
> > > --
> > > 1.7.9.1
>
> --
> Ike
More information about the aur-dev
mailing list