On Mon, Sep 14, 2020 at 09:32:46AM +0200, Martijn van Duren wrote:
> On Mon, 2020-09-14 at 08:12 +0100, Stuart Henderson wrote:
> > On 2020/09/13 22:48, Giovanni Bechis wrote:
> > > "smtpctl spf walk" doesn't work as it should because it breaks when it 
> > > finds
> > > macros as defined in RFC 7208.
> > > 
> > > $ echo ryanair.com | smtpctl spf walk
> > > gives no output while dig reply is:
> > > $ dig txt ryanair.com | grep spf
> > > ryanair.com.            17      IN      TXT     "v=spf1 
> > > include:ryanair.com._nspf.vali.email 
> > > include:%{i}._ip.%{h}._ehlo.%{d}._spf.vali.email ~all"
> > 
> > "spf walk" should return a warning or an error in these cases.
> 
> Was already working on that. How about the diff below?
> 
> $ echo ryanair.com | ./smtpctl/obj/smtpctl spf walk
> smtpctl: lookup_record: %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email contains 
> macros and can't be resolved
> > 
> > > Is it worth mentioning in smtpctl in CAVEATS section or somewhere else ?
> > 
> > Maybe in caveats, but if it's there it should be referenced in the
> > description of "spf walk" too, to make it easier to find.
> > 
> > Text something like this?
> > 
> > "SPF records may contain macros which cannot be included in a static list
> > and must be resolved dynamically at connection time.
> > spf walk cannot provide full results in these cases."
> 
> Text reads fine to me. Added to diff below.
> While here I also changed the # to $ so not to give people the
> impression it should be run as root.
> 
> OK?
> 
> martijn@
> 
> Index: spfwalk.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/spfwalk.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 spfwalk.c
> --- spfwalk.c 15 Mar 2020 16:34:57 -0000      1.17
> +++ spfwalk.c 14 Sep 2020 07:31:03 -0000
> @@ -118,6 +118,11 @@ lookup_record(int type, const char *reco
>       struct asr_query *as;
>       struct target *ntgt;
>  
> +     if (strchr(record, '%') != NULL) {
> +             warnx("%s: %s contains macros and can't be resolved", __func__,
> +                 record);
> +             return;
> +     }

maybe escape the output before it reachs the terminal ? the string
comes from untrusted source...

>       as = res_query_async(record, C_IN, type, NULL);
>       if (as == NULL)
>               err(1, "res_query_async");
> Index: smtpctl.8
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/smtpctl.8,v
> retrieving revision 1.64
> diff -u -p -r1.64 smtpctl.8
> --- smtpctl.8 18 Sep 2018 06:21:45 -0000      1.64
> +++ smtpctl.8 14 Sep 2020 07:31:03 -0000
> @@ -247,8 +247,12 @@ Shows if MTA, MDA and SMTP systems are c
>  Recursively look up SPF records for the domains read from stdin.
>  For example:
>  .Bd -literal -offset indent
> -# smtpctl spf walk < domains.txt
> +$ smtpctl spf walk < domains.txt
>  .Ed
> +.Pp
> +SPF records may contain macros which cannot be included in a static list and
> +must be resolved dynamically at connection time.
> +spf walk cannot provide full results in these cases.
>  .It Cm trace Ar subsystem
>  Enables real-time tracing of
>  .Ar subsystem .
> 

-- 
Sebastien Marie

Reply via email to