Re: [Dnsmasq-discuss] [PATCH] Add support for unique TFTP root per MAC

[Simon Kelley]

On 10/04/17 00:09, Floris Bos wrote:
> On 04/09/2017 11:28 PM, Simon Kelley wrote:
>> Patch accepted, with one change
>>
>>
>>>   snprintf(daemon->namebuff+oldlen,
>>> sizeof(daemon->namebuff)-oldlen, "%.2x-%.2x-%.2x-%.2x-%.2x-%.2x/",
>>
>> daemon->namebuff is a char *, so sizeof(daemon->namebuff) is 4 or 8 and
>> sizeof(daemon->namebuff)-oldlen is a negative number which is a large
>> positive number when promoted  to unsigned size_t.
> 
> Ah, you are right.
> 
> Somehow I had the impression it was a fixed size array, on which
> sizeof() do would work.
> Probably confused it with the other namebuff in tftp_request ( char
> namebuff[IF_NAMESIZE]; )
> 
> Sorry, and thanks for the commit.
> 
> 

No problem. Security, I have to take responsibility for.  My tip for
sound sleep is not to be responsible for C code which runs as root on
100 million devices :)

Cheers,

Simon.





signature.asc
Description: OpenPGP digital signature
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Re: [Dnsmasq-discuss] [PATCH] Add support for unique TFTP root per MAC

[Floris Bos]

On 04/09/2017 11:28 PM, Simon Kelley wrote:

Patch accepted, with one change



  snprintf(daemon->namebuff+oldlen, sizeof(daemon->namebuff)-oldlen, 
"%.2x-%.2x-%.2x-%.2x-%.2x-%.2x/",


daemon->namebuff is a char *, so sizeof(daemon->namebuff) is 4 or 8 and
sizeof(daemon->namebuff)-oldlen is a negative number which is a large
positive number when promoted  to unsigned size_t.


Ah, you are right.

Somehow I had the impression it was a fixed size array, on which 
sizeof() do would work.
Probably confused it with the other namebuff in tftp_request ( char 
namebuff[IF_NAMESIZE]; )


Sorry, and thanks for the commit.



Yours sincerely,

Floris Bos

___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Re: [Dnsmasq-discuss] [PATCH] Add support for unique TFTP root per MAC

[Simon Kelley]

Patch accepted, with one change


> snprintf(daemon->namebuff+oldlen, sizeof(daemon->namebuff)-oldlen, 
> "%.2x-%.2x-%.2x-%.2x-%.2x-%.2x/",


daemon->namebuff is a char *, so sizeof(daemon->namebuff) is 4 or 8 and
sizeof(daemon->namebuff)-oldlen is a negative number which is a large
positive number when promoted  to unsigned size_t. There's thus
effectively no protection here against buffer overflow.

In such ways are security CVEs seeded :)

A changed sizeof(daemon->namebuff) to (MAXDNAME-1) which is the
buffer-size limit used elsewhere in this code.

Cheers,

Simon.



___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

[Dnsmasq-discuss] [PATCH] Add support for unique TFTP root per MAC

[Floris Bos]

It is currently only possible to let the TFTP server serve a different
folder depending on the client's IP address.
However it isn't always possible to predict what the client's
IP address will be, especially in situations in which we are not
responsible for handing them out (e.g. proxy dhcp setups).

Extend the current --tftp-unique-root parameter to support having a
separate folder per MAC address instead.

Signed-off-by: Floris Bos 
---
 man/dnsmasq.8 | 16 ++--
 src/dnsmasq.h |  5 +++--
 src/option.c  | 13 +++--
 src/tftp.c| 34 +-
 4 files changed, 57 insertions(+), 11 deletions(-)

diff --git a/man/dnsmasq.8 b/man/dnsmasq.8
index 05f800c..787c104 100644
--- a/man/dnsmasq.8
+++ b/man/dnsmasq.8
@@ -1814,12 +1814,16 @@ directory is only used for TFTP requests via that 
interface.
 .B --tftp-no-fail
 Do not abort startup if specified tftp root directories are inaccessible.
 .TP
-.B --tftp-unique-root
-Add the IP address of the TFTP client as a path component on the end
-of the TFTP-root (in standard dotted-quad format). Only valid if a
-tftp-root is set and the directory exists. For instance, if tftp-root is 
"/tftp" and client 
-1.2.3.4 requests file "myfile" then the effective path will be
-"/tftp/1.2.3.4/myfile" if /tftp/1.2.3.4 exists or /tftp/myfile otherwise.
+.B --tftp-unique-root[=ip|mac]
+Add the IP or hardware address of the TFTP client as a path component on the 
end
+of the TFTP-root. Only valid if a tftp-root is set and the directory exists.
+Defaults to adding IP address (in standard dotted-quad format).
+For instance, if tftp-root is "/tftp" and client 1.2.3.4 requests file "myfile"
+then the effective path will be "/tftp/1.2.3.4/myfile" if /tftp/1.2.3.4 exists 
or /tftp/myfile otherwise.
+When "=mac" is specified it will append the MAC address instead, using 
lowercase zero padded digits
+separated by dashes, e.g.: 01-02-03-04-aa-bb
+Note that resolving MAC addresses is only possible if the client is in the 
local network or obtained
+a DHCP lease from us.
 .TP
 .B --tftp-secure
 Enable TFTP secure mode: without this, any file which is readable by
diff --git a/src/dnsmasq.h b/src/dnsmasq.h
index 40f249f..25e4ad9 100644
--- a/src/dnsmasq.h
+++ b/src/dnsmasq.h
@@ -211,7 +211,7 @@ struct event_desc {
 #define OPT_TFTP_SECURE26
 #define OPT_TFTP_NOBLOCK   27
 #define OPT_LOG_OPTS   28
-#define OPT_TFTP_APREF 29
+#define OPT_TFTP_APREF_IP  29
 #define OPT_NO_OVERRIDE30
 #define OPT_NO_REBIND  31
 #define OPT_ADD_MAC32
@@ -238,7 +238,8 @@ struct event_desc {
 #define OPT_SCRIPT_ARP 53
 #define OPT_MAC_B6454
 #define OPT_MAC_HEX55
-#define OPT_LAST   56
+#define OPT_TFTP_APREF_MAC 56
+#define OPT_LAST   57
 
 /* extra flags for my_syslog, we use a couple of facilities since they are 
known 
not to occupy the same bits as priorities, no matter how syslog.h is set 
up. */
diff --git a/src/option.c b/src/option.c
index 12350cb..00e1d44 100644
--- a/src/option.c
+++ b/src/option.c
@@ -243,7 +243,7 @@ static const struct myoption opts[] =
 { "enable-tftp", 2, 0, LOPT_TFTP },
 { "tftp-secure", 0, 0, LOPT_SECURE },
 { "tftp-no-fail", 0, 0, LOPT_TFTP_NO_FAIL },
-{ "tftp-unique-root", 0, 0, LOPT_APREF },
+{ "tftp-unique-root", 2, 0, LOPT_APREF },
 { "tftp-root", 1, 0, LOPT_PREFIX },
 { "tftp-max", 1, 0, LOPT_TFTP_MAX },
 { "tftp-mtu", 1, 0, LOPT_TFTP_MTU },
@@ -432,7 +432,7 @@ static struct {
   { LOPT_OVERRIDE, OPT_NO_OVERRIDE, NULL, gettext_noop("Do NOT reuse filename 
and server fields for extra DHCP options."), NULL },
   { LOPT_TFTP, ARG_DUP, "[=[,]]", gettext_noop("Enable integrated 
read-only TFTP server."), NULL },
   { LOPT_PREFIX, ARG_DUP, "[,]", gettext_noop("Export files by 
TFTP only from the specified subtree."), NULL },
-  { LOPT_APREF, OPT_TFTP_APREF, NULL, gettext_noop("Add client IP address to 
tftp-root."), NULL },
+  { LOPT_APREF, ARG_DUP, "[=ip|mac]", gettext_noop("Add client IP or hardware 
address to tftp-root."), NULL },
   { LOPT_SECURE, OPT_TFTP_SECURE, NULL, gettext_noop("Allow access only to 
files owned by the user running dnsmasq."), NULL },
   { LOPT_TFTP_NO_FAIL, OPT_TFTP_NO_FAIL, NULL, gettext_noop("Do not terminate 
the service if TFTP directories are inaccessible."), NULL },
   { LOPT_TFTP_MAX, ARG_ONE, "", gettext_noop("Maximum number of 
concurrent TFTP transfers (defaults to %s)."), "#" },
@@ -2720,6 +2720,15 @@ static int one_opt(int option, char *arg, char *errstr, 
char *gen_err, int comma
} 
   
   break;
+
+case LOPT_APREF: /* --tftp-unique-root */
+  if (!arg || strcasecmp(arg, "ip") == 0)
+set_option_bool(OPT_TFTP_APREF_IP);
+  else if (strcasecmp(arg, "mac") == 0)
+set_option_bool(OPT_TFTP_APREF_MAC);
+  else
+ret_err(gen_err);
+  break;
 #endif
  
 case LOPT_BRIDGE:   /* --bridge-interface */
diff --git