Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[baseimage]: Improve password hashing for default user account #1748

Merged
merged 2 commits into from
Jun 9, 2018

Conversation

serhepopovych
Copy link
Contributor

@serhepopovych serhepopovych commented May 29, 2018

- What I did
Improve password hashing for default user account created during image build.

There are couple of issues with current implementation of default
user account management in baseimage:

  1) It uses DES to encrypt accounts password. Furthermore this
     effectively limits password length to 8 symbols, even if more
     provided with PASSWORD or DEFAULT_PASSWORD from rules/config.

  2) Salt value for password is same on all builds even with different
     password increasing attack surface.

  3) During the build process password passed as command line parameter
     either as plain text (if given to make(1) as "make PASSWORD=...")
     or DES encrypted (if given to build_debian.sh) can be seen by
     non-build users using /proc/<pid>/cmdline file that has group and
     world readable permissions.

- How I did it
To address issues above we propose following changes:

  1) Do not create password by hands (e.g. using perl snippet above):
     put this job to chpasswd(8) which is aware about system wide
     password hashing policy specified in /etc/login.defs with
     ENCRYPT_METHOD (by default it is SHA512 for Debian 8).

  2) Now chpasswd(8) will take care about proper salt value.

  3) This has two steps:

    3.1) For compatibility reasons accept USERNAME and PASSWORD as
         make(1) parameters, but warn user that this is unsafe.

    3.2) Use process environment to pass USERNAME and PASSWORD variables
         from Makefile to build_debian.sh as more secure alternative to
         passing via command line parameters: /proc/<pid>/environ
         readable only by user running process or privileged users like
         root.

- How to verify it
Build image (I use sonic-broadcom.bin), boot it. Use getent(1) tool to get /etc/shadow entry for user.

Before change:
--------------

  hash1
  -----
  # u='admin'
  # p="$(LANG=C perl -e 'print crypt("YourPaSs", "salt"),"\n"')"
                                      ^^^^^^^^
                                      8 symbols
  # echo "$u:$p" | chpasswd -e

  # getent shadow admin
  admin:sazQDkwgZPfSk:17680:0:99999:7:::
        ^^^^^^^^^^^^^
        Note the hash (DES encrypted password)

  hash2
  -----
  # u='admin'
  # p="$(LANG=C perl -e 'print crypt("YourPaSsWoRd", "salt"),"\n"')"
                                      ^^^^^^^^^^^^
                                      12 symbols
  # echo "$u:$p" | chpasswd -e

  # getent shadow admin
  admin:sazQDkwgZPfSk:17680:0:99999:7:::
        ^^^^^^^^^^^^^
        Hash is the same as for "YourPaSs"

After change:
-------------

  hash1
  -----
  # echo "admin:YourPaSs" | chpasswd
  # getent shadow admin
  admin:$6$1Nho1jHC$T8YwK58FYToXMFuetQta7/XouAAN2q1IzWC3bdIg86woAs6WuTg\
           ^^^^^^^^
           Note salt here
  ksLO3oyQInax/wNVq.N4de6dyWZDsCAvsZ1:17681:0:99999:7:::

  hash2
  -----
  # echo "admin:YourPaSs" | chpasswd
  # getent shadow admin
  admin:$6$yKU5g7BO$kdT02Z1wHXhr1VCniKkZbLaMPZXK0WSSVGhSLGrNhsrsVxCJ.D9\
           ^^^^^^^^
           Here salt completely different from case above
  plFpd8ksGNpw/Vb92hvgYyCL2i5cfI8QEY/:17681:0:99999:7:::

Since salt is different hashes for same password different too.

  hash1
  -----
  # LANG=C perl -e 'print crypt("YourPaSs", "\$6\$salt\$"),"\n"'
                                             ^^^^^
                                             We want SHA512 hash
  $6$salt$qkwPvXqUeGpexO1vatnIQFAreOTXs6rnDX.OI.Sz2rcy51JrO8dFc9aGv82bB\
  yd2ELrIMJ.FQLNjgSD0nNha7/

  hash2
  -----
  # LANG=C perl -e 'print crypt("YourPaSsWoRd", "\$6\$salt\$"),"\n"'
  $6$salt$1JVndGzyy/dj7PaXo6hNcttlQoZe23ob8GWYWxVGEiGOlh6sofbaIvwl6Ho7N\
  kYDI8zwRumRwga/A29nHm4mZ1

Now with same "salt" and $<id>$, and same 8 symbol prefix in password, but
different password length we have different hashes.

- Description for the changelog
Improve password hashing for default user account

- A picture of a cute animal (not mandatory but encouraged)

@serhepopovych serhepopovych changed the title Improve password hashing for default user account [baseimage]: Improve password hashing for default user account May 29, 2018
rules/config Outdated
@@ -2,59 +2,63 @@
## Configuration parameters for SONiC build system
###############################################################################

## Note that this file should be valid for inclusion from both Makefile and shell
## scripts. No whitespace allowed between assignment (=) sign, shell script
## compat stuff should go into functions.sh in SNIiC build system root directory.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SNIiC [](start = 47, length = 5)

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reworked code and leave this file as is: no need to source from shell scripts at the moment.

Makefile Outdated
@@ -11,8 +11,6 @@
# * ENABLE_PFCWD_ON_START: Enable PFC Watchdog (PFCWD) on server-facing ports
# * by default for TOR switch.
# * SONIC_ENABLE_SYNCD_RPC: Enables rpc-based syncd builds.
# * USERNAME: Desired username -- default at rules/config
# * PASSWORD: Desired password -- default at rules/config
Copy link
Collaborator

@qiluo-msft qiluo-msft May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PASSWORD [](start = 5, length = 8)

PASSWORD [](start = 5, length = 8)

Could you make the behavior backward compatible. In some case, we are ok to have them in command line. You may add warning for this case.

make PASSWORD=$SONIC_PASSWORD target/sonic-broadcom.bin
``` #Closed

Copy link
Contributor Author

@serhepopovych serhepopovych May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping compatibility is good idea so I leave rules/config in Makefile format as well as opportunity to supply USERNAME/PASSWORD via command line.

Warning message is printed to stderr when passing these variables as make(1) parameters because they will be visible to other users on build host.

Passing USERNAME and PASSWORD from Makefile to build_debian.sh now done via process environment: /proc//environ only available to build user: this has nearly same security level as passing it via configuration file (password will be in process memory anyway, either in environment block or in IO buffers when read from config file).

build_debian.sh Outdated
## Include configuration files
. rules/config

# user
Copy link
Collaborator

@qiluo-msft qiluo-msft May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user [](start = 2, length = 4)

user [](start = 2, length = 4)

Make the comment style consistent? Same below. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer need to include rules/config.

exit 1
}

## Password for the default user, customizable by environment variable
## By default it is an empty password
## You may get a crypted password by: perl -e 'print crypt("YourPaSsWoRd", "salt"),"\n"'
Copy link
Collaborator

@qiluo-msft qiluo-msft May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

salt [](start = 76, length = 4)

Could you give an example?

  1. It uses DES to encrypt accounts password. Furthermore this
    effectively limits password length to 8 symbols, even if more
    provided with PASSWORD or DEFAULT_PASSWORD from rules/config #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I have updated commit description message to contain more information on Before/After change. It should explain problem.

In short: you can try any image and login with only with first 8 symbols from password (e.g. for default image use YourPaSs instead of YourPaSsWoRd).

This works of course only if password for that account isn't changed since image was build (e.g. via passwd(1) or chpasswd(1) tools on running system).

Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As comments.

We display contents of DEFAULT_USERNAME and DEFAULT_PASSWORD, while
image can be build with USERNAME and/or PASSWORD given on make(1)
command line. For example:

  $ make USERNAME=adm PASSWORD=mypass target/sonic-broadcom.bin

Fix by displaying USERNAME and PASSWORD variables in build summary.

Signed-off-by: Sergey Popovich <sergey.popovich@ordnance.co>
slave.mk Outdated
@@ -482,10 +482,14 @@ $(addprefix $(TARGET_PATH)/, $(SONIC_INSTALLERS)) : $(TARGET_PATH)/% : \
chmod +x sonic_debian_extension.sh,
)

DIRTY_SUFFIX="$(shell date +%Y%m%d\.%H%M%S)"
export DIRTY_SUFFIX
Copy link
Collaborator

@qiluo-msft qiluo-msft May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export [](start = 1, length = 6)

export [](start = 1, length = 6)

I think export is needed. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Will respin.
Thanks.

Makefile Outdated
@@ -84,6 +84,12 @@ SONIC_BUILD_INSTRUCTION := make \
@docker inspect --type image $(SLAVE_IMAGE):$(SLAVE_TAG) &> /dev/null || \
{ echo Image $(SLAVE_IMAGE):$(SLAVE_TAG) not found. Building... ; \
$(DOCKER_BUILD) ; }
ifneq ($(USERNAME),)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ifneq [](start = 0, length = 5)

Could you check and warn inside slave.mk?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure. Replace echo with $(warning ...) and improve warning message text. Looks much better now.

However if someone adds USERNAME and PASSWORD to rules/config they will get false positive warning. But I think this isn't big trouble.

There are couple of issues with current implementation of default
user account management in baseimage:

  1) It uses DES to encrypt accounts password. Furthermore this
     effectively limits password length to 8 symbols, even if more
     provided with PASSWORD or DEFAULT_PASSWORD from rules/config.

  2) Salt value for password is same on all builds even with different
     password increasing attack surface.

  3) During the build process password passed as command line parameter
     either as plain text (if given to make(1) as "make PASSWORD=...")
     or DES encrypted (if given to build_debian.sh) can be seen by
     non-build users using /proc/<pid>/cmdline file that has group and
     world readable permissions.

Both 1) and 2) come from:

  perl -e 'print crypt("$(PASSWORD)", "salt"),"\n"')"

that by defalt uses DES if salt does not have format $<id>$<salt>$,
where <id> is hashing function id. See crypt(3) for more details on
valid <id> values.

To address issues above we propose following changes:

  1) Do not create password by hands (e.g. using perl snippet above):
     put this job to chpasswd(8) which is aware about system wide
     password hashing policy specified in /etc/login.defs with
     ENCRYPT_METHOD (by default it is SHA512 for Debian 8).

  2) Now chpasswd(8) will take care about proper salt value.

  3) This has two steps:

    3.1) For compatibility reasons accept USERNAME and PASSWORD as
         make(1) parameters, but warn user that this is unsafe.

    3.2) Use process environment to pass USERNAME and PASSWORD variables
         from Makefile to build_debian.sh as more secure alternative to
         passing via command line parameters: /proc/<pid>/environ
         readable only by user running process or privileged users like
         root.

Before change:
--------------

  hash1
  -----
  # u='admin'
  # p="$(LANG=C perl -e 'print crypt("YourPaSs", "salt"),"\n"')"
                                      ^^^^^^^^
                                      8 symbols
  # echo "$u:$p" | chpasswd -e

  # getent shadow admin
  admin:sazQDkwgZPfSk:17680:0:99999:7:::
        ^^^^^^^^^^^^^
        Note the hash (DES encrypted password)

  hash2
  -----
  # u='admin'
  # p="$(LANG=C perl -e 'print crypt("YourPaSsWoRd", "salt"),"\n"')"
                                      ^^^^^^^^^^^^
                                      12 symbols
  # echo "$u:$p" | chpasswd -e

  # getent shadow admin
  admin:sazQDkwgZPfSk:17680:0:99999:7:::
        ^^^^^^^^^^^^^
        Hash is the same as for "YourPaSs"

After change:
-------------

  hash1
  -----
  # echo "admin:YourPaSs" | chpasswd
  # getent shadow admin
  admin:$6$1Nho1jHC$T8YwK58FYToXMFuetQta7/XouAAN2q1IzWC3bdIg86woAs6WuTg\
           ^^^^^^^^
           Note salt here
  ksLO3oyQInax/wNVq.N4de6dyWZDsCAvsZ1:17681:0:99999:7:::

  hash2
  -----
  # echo "admin:YourPaSs" | chpasswd
  # getent shadow admin
  admin:$6$yKU5g7BO$kdT02Z1wHXhr1VCniKkZbLaMPZXK0WSSVGhSLGrNhsrsVxCJ.D9\
           ^^^^^^^^
           Here salt completely different from case above
  plFpd8ksGNpw/Vb92hvgYyCL2i5cfI8QEY/:17681:0:99999:7:::

Since salt is different hashes for same password different too.

  hash1
  -----
  # LANG=C perl -e 'print crypt("YourPaSs", "\$6\$salt\$"),"\n"'
                                             ^^^^^
                                             We want SHA512 hash
  $6$salt$qkwPvXqUeGpexO1vatnIQFAreOTXs6rnDX.OI.Sz2rcy51JrO8dFc9aGv82bB\
  yd2ELrIMJ.FQLNjgSD0nNha7/

  hash2
  -----
  # LANG=C perl -e 'print crypt("YourPaSsWoRd", "\$6\$salt\$"),"\n"'
  $6$salt$1JVndGzyy/dj7PaXo6hNcttlQoZe23ob8GWYWxVGEiGOlh6sofbaIvwl6Ho7N\
  kYDI8zwRumRwga/A29nHm4mZ1

Now with same "salt" and $<id>$, and same 8 symbol prefix in password, but
different password length we have different hashes.

Signed-off-by: Sergey Popovich <sergey.popovich@ordnance.co>
@lguohan lguohan merged commit 8d88455 into sonic-net:master Jun 9, 2018
@sergeypopovich-ord sergeypopovich-ord deleted the improve-password-hashing branch August 10, 2018 08:03
stepanblyschak added a commit to stepanblyschak/sonic-buildimage that referenced this pull request Nov 1, 2021
```
ca728b8 [config] fix interface IPv6 address removal. (sonic-net#1819)
0665d6f VxLAN Tunnel Counters and Rates implementation (sonic-net#1748)
80a10dc Fix log_ssd_health hang issue (sonic-net#1904)
ea4a730 [config][cbf] Added config commands for CBF (sonic-net#1799)
02ce8d6 [sonic-package-manager] update FEATURE entries on upgrade (sonic-net#1803)
9f123c0 [generate_dump] remove secrets from dump files (sonic-net#1886)
3a8ab73 [fwutil] Add `fwutil update all` to support the automatic platform component fw updates (sonic-net#1242)
776fddf [sonic-package-manager] code style fixes and enhancements (sonic-net#1802)
f53baac [watermarkstat] Fix for error in processing empty array from couters db (sonic-net#1810)
0b2536b Generic_upater: Apply JSON change (sonic-net#1856)
```

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
liat-grozovik pushed a commit that referenced this pull request Nov 2, 2021
```
ca728b8 [config] fix interface IPv6 address removal. (#1819)
0665d6f VxLAN Tunnel Counters and Rates implementation (#1748)
80a10dc Fix log_ssd_health hang issue (#1904)
ea4a730 [config][cbf] Added config commands for CBF (#1799)
02ce8d6 [sonic-package-manager] update FEATURE entries on upgrade (#1803)
9f123c0 [generate_dump] remove secrets from dump files (#1886)
3a8ab73 [fwutil] Add `fwutil update all` to support the automatic platform component fw updates (#1242)
776fddf [sonic-package-manager] code style fixes and enhancements (#1802)
f53baac [watermarkstat] Fix for error in processing empty array from couters db (#1810)
0b2536b Generic_upater: Apply JSON change (#1856)
```

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
theasianpianist pushed a commit to theasianpianist/sonic-buildimage that referenced this pull request Feb 5, 2022
What I did
Add support in sonic-swss for cisco-8000 to detect and recover the port/queue from pfc-wd events.

**Why I did it
Support for cisco-8000 platform for pfc-wd is missing in SONiC

How I verified it
interoperability with a Hardware based Traffic Generator to detect and recover the port/queue using the pfc-wd capability

Details if related

The support does the following:

Uses SAI queue attributes for PFC-WD detection and recovery action
Queues monitored querying SAI_QUEUE_ATTR_PAUSE_STATUS attribute on PFC-WD config
On WD detection, initiate recovery using SAI_QUEUE_ATTR_PFC_DLR_INIT
: Create PfcWdSaiDlrInitHandler object
: ASIC configured to DROP packets for the queue
When queue is out of pause state for restoration_time, restore queue state
: ASIC configured to NOT DROP packets for the queue
: Destroy PfcWdSaiDlrInitHandler object

Signed-off-by: Venkat Garigipati <venkatg@cisco.com>
theasianpianist pushed a commit to theasianpianist/sonic-buildimage that referenced this pull request Feb 5, 2022
…00 (sonic-net#2041)

What I did

Enables support for Rx traffic drop for a port/tc by applying the zero buffer profile
Changed class hierarchy so that default getHwCounters() function is used (since the Rx counter support is enabled)
Fixes a pfc-wd off by 1 detection in case of PFC-WD detection without traffic
Why I did it

enabling a missing functionality
leveraging sonic code as our sdk now implements the missing counter
fixes a bug
How I verified it
on a cisco-8000 router:

detected pfc-wd detection and restore happens within the (detection/restore-time + 1 poll interval) duration and that the changeset passes MSFT tests
Verified that when PFC-WD is detected, both Rx and Tx traffic for a port/tc is dropped and no forwarding happens
Details if related

this patch will need to be double committed to the 202012 branch along with sonic-net#1942 , sonic-net#1748 and sonic-net#1962

Signed-off-by: Alpesh S Patel <alpesh@cisco.com>
taras-keryk pushed a commit to taras-keryk/sonic-buildimage that referenced this pull request Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants