On Wed, 2016-05-11 at 14:54 +0200, Hector Marco-Gisbert wrote: > > El 21/04/16 a las 00:12, Kees Cook escribió: > > On Tue, Apr 19, 2016 at 11:55 AM, Hector Marco-Gisbert <hecmargi@up > > v.es> wrote: > > > > On Wed, Apr 6, 2016 at 12:07 PM, Hector Marco-Gisbert <hecmargi > > > > @upv.es> wrote: > > > > > The minimum address that a process is allowed to mmap when > > > > > LSM is > > > > > enabled is 0x10000 (65536). This value is tunable and > > > > > exported via > > > > > /proc/sys/vm/mmap_min_addr but it is not honored with the > > > > > actual > > > > > minimum value. > > > > > > > > I think this is working as intended already, based on the > > > > commit log > > > > for 788084aba2ab7348257597496befcbccabdc98a3 > > > > > > > > See cap_mmap_addr (which uses dac_mmap_min_addr) vs SELinux's > > > > hook > > > > (which uses CONFIG_LSM_MMAP_MIN_ADDR), and everything else > > > > (that uses > > > > mmap_min_addr). > > > > > > > > Without CONFIG_LSM_MMAP_MIN_ADDR, dac_mmap_min_addr always == > > > > mmap_min_addr. > > > > > > > > With CONFIG_LSM_MMAP_MIN_ADDR, dac_mmap_min_addr can be less > > > > than > > > > mmap_min_addr, but mmap_min_addr will always be at least > > > > CONFIG_LSM_MMAP_MIN_ADDR. > > > > > > > > Eric may be able to shed more light on this... > > > > > > > > -Kees > > > > > > Ok, I see your point, but it seems that minimum address that a > > > process is > > > allowed to map is mmap_min_addr and not dac_mmap_min_addr. > > > This is because mmap_min_addr can be seen as the > > > max(dac_mmap_min_addr, > > > CONFIG_LSM_MMAP_MIN_ADDR) which is correct (the minimum allowed > > > address) but > > > /proc/sys/vm/mmap_min_addr contains dac_mmap_min_addr which is > > > not the minimum. > > > > > > For example, if we set the CONFIG_LSM_MMAP_MIN_ADDR to 65536 and > > > /proc/sys/vm/mmap_min_addr to 4096, then assuming that > > > selinux_mmap_addr() has > > > no permissions (it returns !=0), the minimum allowed address is > > > 65536 not 4096. > > > The mmap check is done in the security_mmap_addr(addr) function > > > in mm/mmap.c > > > file. It seems that we are exporting the dac_mmap_min_addr > > > instead of the actual > > > minimum. > > > > > > Is this behavior intended ? I'm missing something here ? > > Yes, the sysctl is reporting the dac value. > > I think the meaning of the exported mmap_min_addr value was changed > in the > commit you pointed. A new variable was added (dac_mmap_min_addr) and > it was > replaced in the sysctl of "mmap_min_addr" but the exported name > (/proc/sys/vm/mmap_min_addr) was not changed: > > .procname = "mmap_min_addr", > - .data = &mmap_min_addr, > + .data = &dac_mmap_min_addr, > > This can be confusing since the returned value is not the expected > one (the > minimum value according to sysctl/vm.txt) but the dac_mmap_min_addr. > So, I think > that If we need to export the dac value then we can do it but it > would be > desirable not to change the meaning of this exported value. > > Maybe by renaming /proc/sys/vm/mmap_min_addr to > /proc/sys/vm/dac_mmap_min_addr > and adding a read-only /proc/sys/vm/mmap_min_addr ?
This breaks scripts which are currently setting mmap_min_addr (like wine on ubuntu I think?). Seems like a non-starter. You're trying to represent multiple values in a single value. It just isn't possible. You could expose lsm_mmap_min_addr RO in another sysctl (not sure of other places we expose kernel configs like that, but you could). I wouldn't say the meaning of mmap_min_addr changed, we just grew a new (underdocumented) lsm_mmap_min_addr. mmap_min_addr continued to be controlled by and controlling exactly the same thing. dac_mmap_min_addr is controlled by capabilities. lsm_mmap_min_addr is controlled by your LSM. You can expose those 2 values. But it would be us to each process to know how to use them. A process might be able to avoid the dac check but not the mac check (aka a root process) or a process might be able to avoid the mac check but not the dac check (wine). No single value can represent this. The best you could do is expose the lsm/mac value, but I'm not sure I see the value. All you are doing is telling exploit authors exactly how high they have to put their nasty bits... > > If ok, I could send a patch. > > In any case, I think we should update the doc (sysctl/vm.txt). > > All these issue came to light because we are working on a new ASLR > for userspace > and for testing it would be easier if we know where the VMA starts > (this can be > changed at runtime and it affects to the available entropy). > > > Best, > Hector. > > > > > I think it is -- the minimum is correct, it's just that the sysctl > > may > > be reporting the dac value. Eric, are you able to chime in on this? > > > > -Kees > > > > > > > > Thanks, > > > Hector. > > > > > > > > > > > > > > > > > It can be easily checked in a system typing: > > > > > > > > > > $ cat /proc/sys/vm/mmap_min_addr > > > > > 4096 # <= Incorrect, it should be 65536 > > > > > > > > > > $ echo 1024 > /proc/sys/vm/mmap_min_addr > > > > > $ cat /proc/sys/vm/mmap_min_addr > > > > > 1024 # <= Incorrect, it should be 65536 > > > > > > > > > > After applying the patch: > > > > > > > > > > $ cat /proc/sys/vm/mmap_min_addr > > > > > 65536 # <= It is correct > > > > > > > > > > $ echo 1024 > /proc/sys/vm/mmap_min_addr > > > > > $ cat /proc/sys/vm/mmap_min_addr > > > > > 65536 # <= It is correct > > > > > > > > > > > > > > > > > > > > Signed-off-by: Hector Marco-Gisbert <hecma...@upv.es> > > > > > Acked-by: Ismael Ripoll Ripoll <irip...@upv.es> > > > > > --- > > > > > security/min_addr.c | 6 ++++-- > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/security/min_addr.c b/security/min_addr.c > > > > > index f728728..96d1811 100644 > > > > > --- a/security/min_addr.c > > > > > +++ b/security/min_addr.c > > > > > @@ -15,10 +15,12 @@ unsigned long dac_mmap_min_addr = > > > > > CONFIG_DEFAULT_MMAP_MIN_ADDR; > > > > > static void update_mmap_min_addr(void) > > > > > { > > > > > #ifdef CONFIG_LSM_MMAP_MIN_ADDR > > > > > - if (dac_mmap_min_addr > CONFIG_LSM_MMAP_MIN_ADDR) > > > > > + if (dac_mmap_min_addr > CONFIG_LSM_MMAP_MIN_ADDR) { > > > > > mmap_min_addr = dac_mmap_min_addr; > > > > > - else > > > > > + } else { > > > > > mmap_min_addr = CONFIG_LSM_MMAP_MIN_ADDR; > > > > > + dac_mmap_min_addr = CONFIG_LSM_MMAP_MIN_ADDR; > > > > > + } > > > > > #else > > > > > mmap_min_addr = dac_mmap_min_addr; > > > > > #endif > > > > > -- > > > > > 1.9.1 > > > > > > > > > > > > > > > > > > > > > > > -- > > > Dr. Hector Marco-Gisbert @ http://hmarco.org/ > > > Cyber Security Researcher @ http://cybersecurity.upv.es > > > Universitat Politècnica de València (Spain) > > > > > > >