diff mbox

Patch for random mac address

Message ID CAFwXZv_Nb5Q8n1kobbxz7+3w4r6pt-fD5YR4jw6_ZeXk1__9+A@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

HacKurx May 26, 2017, 7:55 a.m. UTC 
2017-05-25 19:28 GMT+02:00 Kees Cook <keescook@chromium.org>:
> On Thu, May 25, 2017 at 8:59 AM, Rik van Riel <riel@redhat.com> wrote:
>> On Thu, 2017-05-25 at 17:47 +0200, intrigeri wrote:
>>> Rik van Riel:
>>> > That suggests maybe this kind of functionality should
>>> > be implemented in userspace, instead?
>>> > Maybe in NetworkManager, […]
>>>
>>> It's already implemented in NetworkManager :)
>>
>> So this kernel patch does not solve any problem,
>> because the solution has already been implemented
>> in userspace?
>
> It makes sure you can never not randomize the MAC

You have perfectly understood.

> BTW, the proposed patch is slightly wrong: it still allows userspace
> to change the MAC address.

This is not the most important because it is already another MAC
address because this patch randomize MAC whenever interface is brought
up.

> The ifdef with the return 0 should be moved
> up (and "return 0" seems like a bit of a lie: maybe -EINVAL or
> -ENOTSUPPORTED?).
-EINVAL seems to be a good idea, I will use it to never reveal the
permanent MAC address.

> How about sending a v2 with that fixed, inline, etc.

Agree with the V2. I'm not a developer, what do you mean by inline? Send by GIT?
If someone can make him grow in my place I will pay him a beer ^^

Thank you all. Best regards,

HacKurx (Loic)

Comments

Anisse Astier May 26, 2017, 12:34 p.m. UTC | #1 
On Fri, May 26, 2017 at 09:55:37AM +0200, HacKurx wrote:
> 2017-05-25 19:28 GMT+02:00 Kees Cook <keescook@chromium.org>:
> > How about sending a v2 with that fixed, inline, etc.
> 
> Agree with the V2. I'm not a developer, what do you mean by inline? Send by GIT?
> If someone can make him grow in my place I will pay him a beer ^^

What he meant, is to follow the kernel process for sending patches.
Lukas already sent a link to the documentation, but you may be
interested in these parts:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#no-mime-no-links-no-compression-no-attachments-just-plain-text
Yes, you can use git as a client (with git send-email), but it's not
mandatory. You should use your real name though:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

And add a Signed-off-by line.


> 
> Thank you all. Best regards,
> 
> HacKurx (Loic)

> diff --git a/net/core/dev.c b/net/core/dev.c
> index fca407b..3eeb42b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6669,6 +6669,26 @@ int dev_change_flags(struct net_device *dev, unsigned int flags)
>  
>  	changes = (old_flags ^ dev->flags) | (old_gflags ^ dev->gflags);
>  	__dev_notify_flags(dev, old_flags, changes);
> +
> +#ifdef CONFIG_RANDOM_MAC_ADDRESS
> +	if ((changes & IFF_UP) && !(old_flags & IFF_UP)) {
> +		/* randomize MAC whenever interface is brought up */
> +		struct sockaddr sa;
> +		unsigned int mac4;
> +		unsigned short mac2;
> +
> +		mac4 = prandom_u32();
> +		mac2 = prandom_u32();
> +		memcpy(sa.sa_data, &mac4, sizeof(mac4));
> +		memcpy((char *)sa.sa_data + sizeof(mac4), &mac2, sizeof(mac2));
> +		if (!is_valid_ether_addr(sa.sa_data))
> +			sa.sa_data[5] = 1;
> +		sa.sa_data[0] &= 0xFC;
> +		sa.sa_family = dev->type;
> +		dev_set_mac_address(dev, &sa);

You didn't answer my question regarding why this is different from the
function eth_random_addr.

Regards,

Anisse
HacKurx May 26, 2017, 2:41 p.m. UTC | #2 
2017-05-26 14:34 GMT+02:00 Anisse Astier <anisse@astier.eu>:
> On Fri, May 26, 2017 at 09:55:37AM +0200, HacKurx wrote:
>> 2017-05-25 19:28 GMT+02:00 Kees Cook <keescook@chromium.org>:
>> > How about sending a v2 with that fixed, inline, etc.
>>
>> Agree with the V2. I'm not a developer, what do you mean by inline? Send by GIT?
>> If someone can make him grow in my place I will pay him a beer ^^
>
> What he meant, is to follow the kernel process for sending patches.
> Lukas already sent a link to the documentation, but you may be
> interested in these parts:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#no-mime-no-links-no-compression-no-attachments-just-plain-text
> Yes, you can use git as a client (with git send-email), but it's not
> mandatory. You should use your real name though:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
>
> And add a Signed-off-by line.

I didn't understand it all that well. English is not my native language.

In your link I could see:
"Gmail (Web GUI)
Does not work for sending patches.
Gmail web client converts tabs to spaces automatically.
At the same time it wraps lines every 78 chars with CRLF style line
breaks although tab2space problem can be solved with external editor.
Another problem is that Gmail will base64-encode any message that has
a non-ASCII character. That includes things like European names."

So here is my error ^^

>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index fca407b..3eeb42b 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -6669,6 +6669,26 @@ int dev_change_flags(struct net_device *dev, unsigned int flags)
>>
>>       changes = (old_flags ^ dev->flags) | (old_gflags ^ dev->gflags);
>>       __dev_notify_flags(dev, old_flags, changes);
>> +
>> +#ifdef CONFIG_RANDOM_MAC_ADDRESS
>> +     if ((changes & IFF_UP) && !(old_flags & IFF_UP)) {
>> +             /* randomize MAC whenever interface is brought up */
>> +             struct sockaddr sa;
>> +             unsigned int mac4;
>> +             unsigned short mac2;
>> +
>> +             mac4 = prandom_u32();
>> +             mac2 = prandom_u32();
>> +             memcpy(sa.sa_data, &mac4, sizeof(mac4));
>> +             memcpy((char *)sa.sa_data + sizeof(mac4), &mac2, sizeof(mac2));
>> +             if (!is_valid_ether_addr(sa.sa_data))
>> +                     sa.sa_data[5] = 1;
>> +             sa.sa_data[0] &= 0xFC;
>> +             sa.sa_family = dev->type;
>> +             dev_set_mac_address(dev, &sa);
>
> You didn't answer my question regarding why this is different from the
> function eth_random_addr.

Because it's been working for years and I'm not a developer / kernel expert.
"Keep it stupidly simple" Might be a reason?
HacKurx June 9, 2017, noon UTC | #3 
Le 26/05/2017 à 14:34, Anisse Astier a écrit :
> On Fri, May 26, 2017 at 09:55:37AM +0200, HacKurx wrote:
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index fca407b..3eeb42b 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -6669,6 +6669,26 @@ int dev_change_flags(struct net_device *dev, unsigned int flags)
>>  
>>  	changes = (old_flags ^ dev->flags) | (old_gflags ^ dev->gflags);
>>  	__dev_notify_flags(dev, old_flags, changes);
>> +
>> +#ifdef CONFIG_RANDOM_MAC_ADDRESS
>> +	if ((changes & IFF_UP) && !(old_flags & IFF_UP)) {
>> +		/* randomize MAC whenever interface is brought up */
>> +		struct sockaddr sa;
>> +		unsigned int mac4;
>> +		unsigned short mac2;
>> +
>> +		mac4 = prandom_u32();
>> +		mac2 = prandom_u32();
>> +		memcpy(sa.sa_data, &mac4, sizeof(mac4));
>> +		memcpy((char *)sa.sa_data + sizeof(mac4), &mac2, sizeof(mac2));
>> +		if (!is_valid_ether_addr(sa.sa_data))
>> +			sa.sa_data[5] = 1;
>> +		sa.sa_data[0] &= 0xFC;
>> +		sa.sa_family = dev->type;
>> +		dev_set_mac_address(dev, &sa);
> You didn't answer my question regarding why this is different from the
> function eth_random_addr.
What do you think by replacing the whole by that?

+#ifdef CONFIG_RANDOM_MAC_ADDRESS
+    /* randomize MAC whenever interface is brought up */
+    if ((changes & IFF_UP) && !(old_flags & IFF_UP)) {
+        struct sockaddr sa;
+        eth_random_addr(sa.sa_data);
+        sa.sa_family = dev->type;
+        dev_set_mac_address(dev, &sa);

The network doesn't work with "eth_hw_addr_random(dev);" (the change of MAC addresses works well). Do you know why ?
Because the eth_hw_addr_randomfunction works better on all types of network cards.

Thanks,

Loic
Anisse Astier June 9, 2017, 1:01 p.m. UTC | #4 
On Fri, Jun 09, 2017 at 02:00:37PM +0200, HacKurx wrote:
> Le 26/05/2017 à 14:34, Anisse Astier a écrit :
> > You didn't answer my question regarding why this is different from the
> > function eth_random_addr.
> What do you think by replacing the whole by that?
> 
> +#ifdef CONFIG_RANDOM_MAC_ADDRESS
> +    /* randomize MAC whenever interface is brought up */
> +    if ((changes & IFF_UP) && !(old_flags & IFF_UP)) {
> +        struct sockaddr sa;
> +        eth_random_addr(sa.sa_data);
> +        sa.sa_family = dev->type;
> +        dev_set_mac_address(dev, &sa);

Yes, I meant something like that.

> 
> The network doesn't work with "eth_hw_addr_random(dev);" (the change of MAC addresses works well). Do you know why ?

I think that's because eth_hw_addr_random() is meant to be called by the
driver. Your solution is simpler, although it doesn't set
addr_assign_type to NET_ADDR_RANDOM, athough this field seems to be used
erratically (some use it as bitfield, but it does not look like one).

> Because the eth_hw_addr_randomfunction works better on all types of network cards.

You mean eth_random_addr, correct ?

Regards,

Anisse
diff mbox

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index fca407b..3eeb42b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6669,6 +6669,26 @@  int dev_change_flags(struct net_device *dev, unsigned int flags)
 
 	changes = (old_flags ^ dev->flags) | (old_gflags ^ dev->gflags);
 	__dev_notify_flags(dev, old_flags, changes);
+
+#ifdef CONFIG_RANDOM_MAC_ADDRESS
+	if ((changes & IFF_UP) && !(old_flags & IFF_UP)) {
+		/* randomize MAC whenever interface is brought up */
+		struct sockaddr sa;
+		unsigned int mac4;
+		unsigned short mac2;
+
+		mac4 = prandom_u32();
+		mac2 = prandom_u32();
+		memcpy(sa.sa_data, &mac4, sizeof(mac4));
+		memcpy((char *)sa.sa_data + sizeof(mac4), &mac2, sizeof(mac2));
+		if (!is_valid_ether_addr(sa.sa_data))
+			sa.sa_data[5] = 1;
+		sa.sa_data[0] &= 0xFC;
+		sa.sa_family = dev->type;
+		dev_set_mac_address(dev, &sa);
+	}
+#endif
+
 	return ret;
 }
 EXPORT_SYMBOL(dev_change_flags);
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index b94b1d2..b020d15 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -463,7 +463,12 @@ 
 					 sizeof(struct ifreq)))
 				ret = -EFAULT;
 		}
+#ifdef CONFIG_RANDOM_MAC_ADDRESS
+		/* Don't reveal the permanent MAC address */
+		return -EINVAL;
+#else
 		return ret;
+#endif
 
 	/*
 	 *	These ioctl calls:
diff --git a/security/Kconfig b/security/Kconfig
index 93027fd..6b7b6fc 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -67,6 +67,14 @@  config SECURITY_NETWORK_XFRM
 	  IPSec.
 	  If you are unsure how to answer this question, answer N.
 
+config RANDOM_MAC_ADDRESS
+	bool "Use random MAC adresses"
+	default n
+	help
+	  Say Y here for randomize the MAC addresses of network interfaces.
+	  This option is recommended for people who want to increase their privacy.
+	  If you are unsure how to answer this question, answer N.
+
 config SECURITY_PATH
 	bool "Security hooks for pathname based access control"
 	depends on SECURITY