[2/2] Provide PAM default configuration files when building linux-pam package

Signed-off-by: Dmitry <golubovsky@gmail.com>
---
 package/linux-pam/default      |    8 ++++++++
 package/linux-pam/linux-pam.mk |    7 +++++++
 package/linux-pam/login        |    9 +++++++++
 3 files changed, 24 insertions(+), 0 deletions(-)
 create mode 100644 package/linux-pam/default
 create mode 100644 package/linux-pam/login

Dmitry, All,

On Tuesday 04 September 2012 05:28:42 Dmitry wrote:
> Signed-off-by: Dmitry <golubovsky@gmail.com>
> ---
>  package/linux-pam/default      |    8 ++++++++
>  package/linux-pam/linux-pam.mk |    7 +++++++
>  package/linux-pam/login        |    9 +++++++++

I think that this patch, which adds the default files, should come
_before_ the busybox patch. If only the busybox patch were to be
applied, then PAM would not be useable as it would lack those files.

In the current order, iIt would also break 'bisectability'.

>  3 files changed, 24 insertions(+), 0 deletions(-)
>  create mode 100644 package/linux-pam/default
>  create mode 100644 package/linux-pam/login
> 
> diff --git a/package/linux-pam/default b/package/linux-pam/default
> new file mode 100644
> index 0000000..0bd5ba0
> --- /dev/null
> +++ b/package/linux-pam/default
> @@ -0,0 +1,8 @@
> +#
> +# default; standard UN*X access
> +#
> +auth     required       pam_unix.so
> +account  required       pam_unix.so
> +password required       pam_unix.so
> +session  required       pam_unix.so
> +

I am not a PAM expert, so I can't say whether these settings are correct,
enough, or whatever. I'd trust close to anybody on this subject. ;-)

> diff --git a/package/linux-pam/linux-pam.mk b/package/linux-pam/linux-pam.mk
> index 48cb073..2807bc1 100644
> --- a/package/linux-pam/linux-pam.mk
> +++ b/package/linux-pam/linux-pam.mk
> @@ -24,4 +24,11 @@ ifeq ($(BR2_PACKAGE_LIBINTL),y)
>  	LINUX_PAM_MAKE_OPT += LIBS=-lintl
>  endif
>  
> +define LINUX_PAM_CONFFILES
> +	$(INSTALL) -D -m 0644 package/linux-pam/default $(TARGET_DIR)/etc/pam.d/default
> +	$(INSTALL) -D -m 0644 package/linux-pam/login $(TARGET_DIR)/etc/pam.d/login

I'd use:
    $(INSTALL) -D -m 0644 $(@D)/default $(TARGET_DIR)/etc/pam.d/default

Also, shouldn't these files get special permission (ie. redable only by
root, or stuff like that)? If so, then use:
  LINUX_PAM_PERMISSIONS = .....

> +endef
> +
> +LINUX_PAM_POST_INSTALL_TARGET_HOOKS += LINUX_PAM_CONFFILES
> +
>  $(eval $(autotools-package))
> diff --git a/package/linux-pam/login b/package/linux-pam/login
> new file mode 100644
> index 0000000..d65a9d4
> --- /dev/null
> +++ b/package/linux-pam/login
> @@ -0,0 +1,9 @@
> +#
> +# login: allow local logins to users with entries in /etc/passwd and
> +# /etc/shadow even with null password
> +#
> +auth      required       pam_unix.so nullok
> +account   required       pam_unix.so nullok
> +password  required       pam_unix.so nullok
> +session   required       pam_unix.so nullok

Ditto, I'm not a PAM expert...

Although I doubt I'd like a system where null passwords are OK... :-/
At the risk of adding to the option maze, I'd suggest at least adding
a config knob to enable that. For example:

  config BR2_PACKAGE_LINUX_PAM_NULL_PASSWD_OK
      bool "Allow null passwords"
      help
        Allow local logins to users with entries in /etc/passwd
        and /etc/shadow even with null password.

And only add the "nullok" if that option is set.

If that's not OK to add such an option, then I'd say we should remove the
"nullok" stuff, and leave it to a local post-build script that tweaks this
file if the user really wants to allow local null-password logins.

IMNSHO, the defaut should be a secure system.

Regards,
Yann E. MORIN.

Yann,

Thanks for your comments. Replies below.

On Tue, Sep 4, 2012 at 1:02 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:

>> ---
>>  package/linux-pam/default      |    8 ++++++++
>>  package/linux-pam/linux-pam.mk |    7 +++++++
>>  package/linux-pam/login        |    9 +++++++++
>
> I think that this patch, which adds the default files, should come
> _before_ the busybox patch. If only the busybox patch were to be
> applied, then PAM would not be useable as it would lack those files.
>
> In the current order, iIt would also break 'bisectability'.

This patch was a "reasonable compromise" to have a somewhat working
system emulating PAM-less behavior when PAM is enabled in login and no
proper PAM configs are provided.
I think that in a real project, a post-build filesystem fix should
replace these files with something more sensible (like mine does).
Regarding ordering, I'll try to switch the order.

>> +#
>> +# default; standard UN*X access
>> +#
>> +auth     required       pam_unix.so
>> +account  required       pam_unix.so
>> +password required       pam_unix.so
>> +session  required       pam_unix.so
>> +
>
> I am not a PAM expert, so I can't say whether these settings are correct,
> enough, or whatever. I'd trust close to anybody on this subject. ;-)

This is an example from PAM documentation: in all entries standard
Unix files (passwd/shadow) are checked, and null passwords are not
allowed.

>> +define LINUX_PAM_CONFFILES
>> +     $(INSTALL) -D -m 0644 package/linux-pam/default $(TARGET_DIR)/etc/pam.d/default
>> +     $(INSTALL) -D -m 0644 package/linux-pam/login $(TARGET_DIR)/etc/pam.d/login
>
> I'd use:
>     $(INSTALL) -D -m 0644 $(@D)/default $(TARGET_DIR)/etc/pam.d/default

But $(@D) is the build directory, while the files are part of
Buildroot package rather than PAM itself. $(@D) is to my understanding
output/build/linux-pam-x.y.z

>
> Also, shouldn't these files get special permission (ie. redable only by
> root, or stuff like that)? If so, then use:
>   LINUX_PAM_PERMISSIONS = .....

Not sure if that's needed: they do not have anything secret, just
nobody other than root can change them.

>> +# login: allow local logins to users with entries in /etc/passwd and
>> +# /etc/shadow even with null password
>> +#
>> +auth      required       pam_unix.so nullok
>> +account   required       pam_unix.so nullok
>> +password  required       pam_unix.so nullok
>> +session   required       pam_unix.so nullok
>
> Ditto, I'm not a PAM expert...

Neither am I... Just porting.

>
> Although I doubt I'd like a system where null passwords are OK... :-/

PAM-less mode allows this: the above config applies to local logins
only via busybox login.

> At the risk of adding to the option maze, I'd suggest at least adding
> a config knob to enable that. For example:
>
>   config BR2_PACKAGE_LINUX_PAM_NULL_PASSWD_OK
>       bool "Allow null passwords"
>       help
>         Allow local logins to users with entries in /etc/passwd
>         and /etc/shadow even with null password.
>
> And only add the "nullok" if that option is set.

I'm afraid this is too much. And it should be provided for the
PAM-less config as well.

One who wants to use PAM will likely design a proper authentication
scheme and provide their own better PAM configs.

>
> If that's not OK to add such an option, then I'd say we should remove the
> "nullok" stuff, and leave it to a local post-build script that tweaks this
> file if the user really wants to allow local null-password logins.
>
> IMNSHO, the defaut should be a secure system.

But it is not by default: root can login w/o password

http://git.buildroot.net/buildroot/tree/fs/skeleton/etc/shadow

this is /etc/shadow from buildroot git which is installed by default:
am I correct?

Thanks.

Dmitry, All,

On Tuesday 04 September 2012 19:33:08 Dmitry Golubovsky wrote:
> On Tue, Sep 4, 2012 at 1:02 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > I think that this patch, which adds the default files, should come
> > _before_ the busybox patch. If only the busybox patch were to be
> > applied, then PAM would not be useable as it would lack those files.
> >
> > In the current order, iIt would also break 'bisectability'.
> 
> This patch was a "reasonable compromise" to have a somewhat working
> system emulating PAM-less behavior when PAM is enabled in login and no
> proper PAM configs are provided.
> I think that in a real project, a post-build filesystem fix should
> replace these files with something more sensible (like mine does).

Ah, OK I get it. With these two files, even if no PAM configuration is
provided by other means, it would behave as if PAM was not enabled. Right?

> Regarding ordering, I'll try to switch the order.

Yes, because without this patch, if linux-pam is enabled, and PAM is
enabled in busybox (which is possible with the previous patch), what
would be the behaviour?

I guess that, without its config files, PAM would not allow anything,
right?

If so, then the default files should come _before_ they are required.

> >> +#
> >> +# default; standard UN*X access
> >> +#
> >> +auth     required       pam_unix.so
> >> +account  required       pam_unix.so
> >> +password required       pam_unix.so
> >> +session  required       pam_unix.so
> >> +
> >
> > I am not a PAM expert, so I can't say whether these settings are correct,
> > enough, or whatever. I'd trust close to anybody on this subject. ;-)
> 
> This is an example from PAM documentation:

OK, good! :-)

> >> +define LINUX_PAM_CONFFILES
> >> +     $(INSTALL) -D -m 0644 package/linux-pam/default $(TARGET_DIR)/etc/pam.d/default
> >> +     $(INSTALL) -D -m 0644 package/linux-pam/login $(TARGET_DIR)/etc/pam.d/login
> >
> > I'd use:
> >     $(INSTALL) -D -m 0644 $(@D)/default $(TARGET_DIR)/etc/pam.d/default
> 
> But $(@D) is the build directory, while the files are part of
> Buildroot package rather than PAM itself. $(@D) is to my understanding
> output/build/linux-pam-x.y.z

Gah, my bad...
This morning's cafeine dose is no longer having any effect... :-(

> > Also, shouldn't these files get special permission (ie. redable only by
> > root, or stuff like that)? If so, then use:
> >   LINUX_PAM_PERMISSIONS = .....
> 
> Not sure if that's needed: they do not have anything secret, just
> nobody other than root can change them.

OK, I just checked on my distro, and indeed there're world-readable.
/etc/shadow is not, however, and that was what I probably was thinking
about (damn lack of cafeine is kicking again...)

> > At the risk of adding to the option maze, I'd suggest at least adding
> > a config knob to enable that. For example:
[--SNIP--]
> I'm afraid this is too much. And it should be provided for the
> PAM-less config as well.

Yep.

> One who wants to use PAM will likely design a proper authentication
> scheme and provide their own better PAM configs.

Granted.

> > IMNSHO, the defaut should be a secure system.
> But it is not by default: root can login w/o password

Yep again, indeed.

If you resubmit with the ordering reversed, you can add my:
    Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
for this patch (I'll review the other later).

Thanks for bearing with me. ;-)

Regards,
Yann E. MORIN.

Yann,

On Tue, Sep 4, 2012 at 1:52 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:

>> This patch was a "reasonable compromise" to have a somewhat working
>> system emulating PAM-less behavior when PAM is enabled in login and no
>> proper PAM configs are provided.
>> I think that in a real project, a post-build filesystem fix should
>> replace these files with something more sensible (like mine does).
>
> Ah, OK I get it. With these two files, even if no PAM configuration is
> provided by other means, it would behave as if PAM was not enabled. Right?

Correct: local logins via busybox login (or whatever calls pam_start
with "login" as program name) will be possible for "default" and
"root" (but not any other config) even if /etc/shadow has null
passwords. At least in my tests it worked so.

> I guess that, without its config files, PAM would not allow anything,
> right?

Correct.

>
> If so, then the default files should come _before_ they are required.
>

Yes, this is reasonable.

>
> Thanks for bearing with me. ;-)
>

That's fine: thanks for your comments.

I'll see if there are any more comments on these patches and will try
to reorder and resubmit them later tonight.