bigbluebutton / bbb-install

BASH script to install BigBlueButton in 30 minutes.
GNU Lesser General Public License v3.0
616 stars 539 forks source link

Insecure use of Debian repository keys #134

Open hannob opened 4 years ago

hannob commented 4 years ago

I noticed the bbb-install.sh script adds various thirdparty repositories and corresponding PGP keys [1].

There are a variety of security issues with that, see [2] for some background.

The most problematic part is here:

    if ! apt-key list 5AFA7A83 | grep -q -E "1024|4096"; then   # Add Kurento package
      sudo apt-key adv --keyserver keyserver.ubuntu.com --recv-keys 5AFA7A83

This fetches a key from the ubuntu keyserver based on its short key ID. The connection to the key server is not encrypted. Short key ids can easily be forged, see [3]. Also one could poison this repository key on the keyserver.

I recommend fetching all repository keys from HTTPS locations (like you do with the others). Also I would very generally recommend replacing all short key ids in the script with their full 160 bit key ids. (E.g. instead of 5AFA7A83 use 234821A61B67740F89BFD669FC8A16625AFA7A83, but same for all other key ids in that script.)

[1] https://github.com/bigbluebutton/bbb-install/blob/master/bbb-install.sh

[2] https://blog.hboeck.de/archives/897-Security-Issues-with-PGP-Signatures-and-Linux-Package-Management.html

[3] https://evil32.com/

ffdixon commented 4 years ago

Thanks for your feedback! We were trying to avoid fetching the keys each time.

I recommend fetching all repository keys from HTTPS locations (like you do with the others).

Do you have a suggestion on a better way to check if the key exists (code suggestions would be welcome!)

hannob commented 4 years ago

You mean the grep part here?

if ! apt-key list 5AFA7A83 | grep -q -E "1024|4096"; then

I just tried, but it seems there's no way to let apt-key create a return value, so it's always messy...

So maybe leave it like it is except using the full keyid:

if ! apt-key list 234821A61B67740F89BFD669FC8A16625AFA7A83 | grep -q -E "1024|4096"; then
sualko commented 4 years ago

Is this really a security issue? apt-key list only shows already installed and trusted keys.

need_ppa() {
  need_pkg software-properties-common 
  if [ ! -f /etc/apt/sources.list.d/$1 ]; then
    LC_CTYPE=C.UTF-8 add-apt-repository -y $2 
  fi
  if ! apt-key list $3 | grep -q -E "1024|4096"; then  # Let's try it a second time
    LC_CTYPE=C.UTF-8 add-apt-repository $2 -y
    if ! apt-key list $3 | grep -q -E "1024|4096"; then
      err "Unable to setup PPA for $2"
    fi
  fi
}

I have more issues with the whole function. add-apt-repository should create no duplicates and to check if the operation was successful, the return value should be considered.

hannob commented 4 years ago

@sualko the if clause is probably no security issue, but the --recv-key with the short key id is. But as a rule of thumb: Short key ids aren't unique, and there's no downside in using the full ones (particularly in a script where you don't have to type them). So best to just replace them everywhere to avoid problems.

remil1000 commented 3 years ago

I think the whole import 5AFA7A83 line can be removed in current version (2.3+)

From the bbb-install.sh script that itself indicates kurento is fetched from main BBB repo:

    if ! apt-key list 5AFA7A83 | grep -q -E "1024|4096"; then   # Add Kurento package
      sudo apt-key adv --keyserver keyserver.ubuntu.com --recv-keys 5AFA7A83
    fi

    rm -rf /etc/apt/sources.list.d/kurento.list     # Kurento 6.15 now packaged with 2.3

the installed kurento-media-server as per apt-cache policy comes from https://ubuntu.bigbluebutton.org/bionic-23 bigbluebutton-bionic/main

Key ID of https://ubuntu.bigbluebutton.org/bionic-23 bigbluebutton-bionic/main is 37B5DD5EFAB46452 and updated by the following piece of code

  if ! apt-key list | grep -q "BigBlueButton apt-get"; then
    wget "https://$PACKAGE_REPOSITORY/repo/bigbluebutton.asc" -O- | apt-key add -
  fi

Just got bit by this as I had some http_proxy configuration in place but not for the hkp:// protocol