From e8c3ec6c80c57a20c3c65f6c86e66a75a3c61f37 Mon Sep 17 00:00:00 2001 From: Mykola Nikishov Date: Thu, 1 Sep 2016 12:00:31 +0300 Subject: [PATCH 1/7] Add test for 'el-get-insecure-check --- test/el-get-tests.el | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/el-get-tests.el b/test/el-get-tests.el index 56294ecb..47a60c0b 100644 --- a/test/el-get-tests.el +++ b/test/el-get-tests.el @@ -125,3 +125,33 @@ Following variables are bound to temporal values: (should-not (featurep pkg)) (el-get 'sync (mapcar 'el-get-source-name el-get-sources)) (should (featurep pkg))))) + +(defconst insecure-urls '("http://example.com" + "ftp://example.com" + ":pserver:anonymous@example.com")) + +(ert-deftest el-get-insecure-check-insecure () + "Insecure URL for a package without :checksum" + (dolist (url insecure-urls) + (let ((el-get-allow-insecure nil) + (el-get-sources '((:name "dummy" :type github)))) + ;; TODO check for error message? + (should-error (el-get-insecure-check "dummy" url) :type 'error)))) + +(defconst secure-urls '("https://example.com" + "ssh://example.com" + "John.Doe-123_@example.com")) + +(ert-deftest el-get-insecure-check-secure () + "Secure URL for a package without :checksum doesn't matter" + (dolist (url secure-urls) + (let ((el-get-allow-insecure nil) + (el-get-sources '((:name "dummy" :type github)))) + (should-not (el-get-insecure-check "dummy" url))))) + +(ert-deftest el-get-insecure-check-checksum () + "Either secure or insecure URL for a package with :checksum" + (dolist (url (append insecure-urls secure-urls)) + (let ((el-get-allow-insecure nil) + (el-get-sources '((:name "dummy" :type github :checksum "checksum")))) + (should-not (el-get-insecure-check "dummy" url))))) From 2044e6da7627acd30f78e281e408b4f213ef2ad0 Mon Sep 17 00:00:00 2001 From: Mykola Nikishov Date: Tue, 30 Aug 2016 21:17:59 +0300 Subject: [PATCH 2/7] More clear error message from 'el-get-insecure-check ...because not a package itself but a URL user trying to install it from is actually insecure. Mention URL in the error message like Attempting to install package ag from insecure URL user@ftp://example.com/ without `el-get-allow-insecure for easier troubleshooting. --- el-get-methods.el | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/el-get-methods.el b/el-get-methods.el index e98e8155..f9942e36 100644 --- a/el-get-methods.el +++ b/el-get-methods.el @@ -30,8 +30,9 @@ ;; If we have :checksum, we can rely on `el-get-post-install' for ;; security. (unless (plist-get (el-get-package-def package) :checksum) - (error (concat "Attempting to install insecure package " + (error (concat "Attempting to install package " (el-get-as-string package) + " from insecure URL " url " without `el-get-allow-insecure'."))))) (require 'el-get-apt-get) From 070dddde7ef03f4f92c94e6596434048a44c307c Mon Sep 17 00:00:00 2001 From: Mykola Nikishov Date: Fri, 2 Sep 2016 14:49:06 +0300 Subject: [PATCH 3/7] Consider URL pointing to a local file as secure URL starting with 'file:///' (hostname is empty) is secure because it always points to a local file. OTOH, 'file://example.com/' (with any hostname, including 'localhost' and '127.0.0.1') is insecure as it may refer to the remote file and deciding if some hostname is actually a local in given moment in time is tricky and too error-prone. --- el-get-methods.el | 1 + test/el-get-tests.el | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/el-get-methods.el b/el-get-methods.el index f9942e36..deeec665 100644 --- a/el-get-methods.el +++ b/el-get-methods.el @@ -24,6 +24,7 @@ (defun el-get-insecure-check (package url) (when (and (not el-get-allow-insecure) + (not (string-match "^file:///" url)) (not (string-match "^https://" url)) (not (string-match "^[-_\.A-Za-z0-9]+@" url)) (not (string-match "^ssh" url))) diff --git a/test/el-get-tests.el b/test/el-get-tests.el index 47a60c0b..aad7a474 100644 --- a/test/el-get-tests.el +++ b/test/el-get-tests.el @@ -128,6 +128,7 @@ Following variables are bound to temporal values: (defconst insecure-urls '("http://example.com" "ftp://example.com" + "file://example.com/home/user" ":pserver:anonymous@example.com")) (ert-deftest el-get-insecure-check-insecure () @@ -140,6 +141,9 @@ Following variables are bound to temporal values: (defconst secure-urls '("https://example.com" "ssh://example.com" + "file:///home/user" + "file:///c|/WINDOWS/clock.avi" + "file:///c:/WINDOWS/clock.avi" "John.Doe-123_@example.com")) (ert-deftest el-get-insecure-check-secure () From 17b0da498455328784576e5945de6ad82b469979 Mon Sep 17 00:00:00 2001 From: Mykola Nikishov Date: Thu, 1 Sep 2016 12:01:46 +0300 Subject: [PATCH 4/7] Package with empty :checksum is insecure For compatibility with Emacs versions before 24.4, fall back to 'string-match if 'string-blank-p from subr-x is not available. --- el-get-methods.el | 31 +++++++++++++++++++------------ test/el-get-tests.el | 9 +++++++++ 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/el-get-methods.el b/el-get-methods.el index deeec665..4d626594 100644 --- a/el-get-methods.el +++ b/el-get-methods.el @@ -12,6 +12,8 @@ ;; Install ;; Please see the README.md file from the same distribution (require 'el-get-core) +(unless (version< emacs-version "24.4") + (require 'subr-x)) ;; ;; NOTE: this will probably benefit from some autoloading magic, later. @@ -23,18 +25,23 @@ (file-name-directory (or load-file-name byte-compile-current-file buffer-file-name))))) (defun el-get-insecure-check (package url) - (when (and (not el-get-allow-insecure) - (not (string-match "^file:///" url)) - (not (string-match "^https://" url)) - (not (string-match "^[-_\.A-Za-z0-9]+@" url)) - (not (string-match "^ssh" url))) - ;; If we have :checksum, we can rely on `el-get-post-install' for - ;; security. - (unless (plist-get (el-get-package-def package) :checksum) - (error (concat "Attempting to install package " - (el-get-as-string package) - " from insecure URL " url - " without `el-get-allow-insecure'."))))) + (let* ((checksum (plist-get (el-get-package-def package) :checksum)) + (checksum-empty (or (not (stringp checksum)) + (if (fboundp 'string-blank-p) + (string-blank-p checksum) + (string-match-p "\\`[ \t\n\r]*\\'" checksum))))) + (when (and (not el-get-allow-insecure) + (not (string-match "^file:///" url)) + (not (string-match "^https://" url)) + (not (string-match "^[-_\.A-Za-z0-9]+@" url)) + (not (string-match "^ssh" url))) + ;; With not empty :checksum, we can rely on `el-get-post-install' calling + ;; `el-get-verify-checksum' for security. + (unless (not checksum-empty) + (error (concat "Attempting to install package " + (el-get-as-string package) + " from insecure URL " url + " without `el-get-allow-insecure'.")))))) (require 'el-get-apt-get) (require 'el-get-builtin) diff --git a/test/el-get-tests.el b/test/el-get-tests.el index aad7a474..a6b17e98 100644 --- a/test/el-get-tests.el +++ b/test/el-get-tests.el @@ -159,3 +159,12 @@ Following variables are bound to temporal values: (let ((el-get-allow-insecure nil) (el-get-sources '((:name "dummy" :type github :checksum "checksum")))) (should-not (el-get-insecure-check "dummy" url))))) + +(ert-deftest el-get-insecure-check-checksum-empty () + "Insecure URL for a package with empty :checksum" + (dolist (url insecure-urls) + (dolist (checksum '("" " ")) + (let ((el-get-allow-insecure nil) + (el-get-sources '((:name "dummy" :type github :checksum checksum)))) + ;; TODO check for error message? + (should-error (el-get-insecure-check "dummy" url) :type 'error))))) From 6a724f91688385ae12545271debfbc1111c2b15b Mon Sep 17 00:00:00 2001 From: Mykola Nikishov Date: Tue, 30 Aug 2016 19:07:29 +0300 Subject: [PATCH 5/7] Add few more 'insecure' URLs to the test For some reason, these, X-over-SSH protocols, are not considered as secure by 'el-get-insecure-check: - git+ssh - bzr+ssh - sftp --- test/el-get-tests.el | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/el-get-tests.el b/test/el-get-tests.el index a6b17e98..2e2275a1 100644 --- a/test/el-get-tests.el +++ b/test/el-get-tests.el @@ -129,6 +129,9 @@ Following variables are bound to temporal values: (defconst insecure-urls '("http://example.com" "ftp://example.com" "file://example.com/home/user" + "git+ssh://example.com/" + "bzr+ssh://example.com/" + "sftp://example.com/" ":pserver:anonymous@example.com")) (ert-deftest el-get-insecure-check-insecure () From 600ddcee7605f1ccadf7e3ad944aac3e9f040bfe Mon Sep 17 00:00:00 2001 From: Mykola Nikishov Date: Sat, 14 May 2016 14:05:11 +0300 Subject: [PATCH 6/7] Treat 'bzr+ssh', 'git+ssh' and 'sftp' as secure protocols --- el-get-methods.el | 3 +++ test/el-get-tests.el | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/el-get-methods.el b/el-get-methods.el index 4d626594..5a5e6b1f 100644 --- a/el-get-methods.el +++ b/el-get-methods.el @@ -34,6 +34,9 @@ (not (string-match "^file:///" url)) (not (string-match "^https://" url)) (not (string-match "^[-_\.A-Za-z0-9]+@" url)) + (not (string-match "^sftp://" url)) + (not (string-match "^bzr\\+ssh://" url)) + (not (string-match "^git\\+ssh://" url)) (not (string-match "^ssh" url))) ;; With not empty :checksum, we can rely on `el-get-post-install' calling ;; `el-get-verify-checksum' for security. diff --git a/test/el-get-tests.el b/test/el-get-tests.el index 2e2275a1..1a295eb5 100644 --- a/test/el-get-tests.el +++ b/test/el-get-tests.el @@ -129,9 +129,6 @@ Following variables are bound to temporal values: (defconst insecure-urls '("http://example.com" "ftp://example.com" "file://example.com/home/user" - "git+ssh://example.com/" - "bzr+ssh://example.com/" - "sftp://example.com/" ":pserver:anonymous@example.com")) (ert-deftest el-get-insecure-check-insecure () @@ -144,6 +141,9 @@ Following variables are bound to temporal values: (defconst secure-urls '("https://example.com" "ssh://example.com" + "git+ssh://example.com/" + "bzr+ssh://example.com/" + "sftp://example.com/" "file:///home/user" "file:///c|/WINDOWS/clock.avi" "file:///c:/WINDOWS/clock.avi" From cb02c7118141f1c7c0a832c778d8de6530f6e32a Mon Sep 17 00:00:00 2001 From: Mykola Nikishov Date: Mon, 5 Sep 2016 06:51:05 +0300 Subject: [PATCH 7/7] Customize list of secure URL protocols via 'el-get-secure-protocols --- el-get-custom.el | 11 ++++++++++- el-get-methods.el | 36 ++++++++++++++++++++++++------------ test/el-get-tests.el | 8 +++++++- 3 files changed, 41 insertions(+), 14 deletions(-) diff --git a/el-get-custom.el b/el-get-custom.el index 40955420..a64ba8d9 100644 --- a/el-get-custom.el +++ b/el-get-custom.el @@ -614,8 +614,17 @@ platforms where this recipe should apply" ;; TODO: this should be nil; change at the next major version bump (defcustom el-get-allow-insecure t - "Allow packages to be installed over insecure connections." + "Allow packages to be installed over insecure connections. + +See `el-get-insecure-check'." :group 'el-get :type 'boolean) +(defcustom el-get-secure-protocols '("https" "ssh" "git+ssh" "bzr+ssh" "sftp") + "List of secure protocols. + +See `el-get-insecure-check'." + :group 'el-get + :type '(repeat string)) + (provide 'el-get-custom) diff --git a/el-get-methods.el b/el-get-methods.el index 5a5e6b1f..80f99bc7 100644 --- a/el-get-methods.el +++ b/el-get-methods.el @@ -24,26 +24,38 @@ "methods" (file-name-directory (or load-file-name byte-compile-current-file buffer-file-name))))) -(defun el-get-insecure-check (package url) - (let* ((checksum (plist-get (el-get-package-def package) :checksum)) +(defun el-get-insecure-check (PACKAGE URL) + "Raise an error if it's not safe to install PACKAGE from URL. + +When `el-get-allow-insecure' is non-nil, check if either of the +following is true and retun nil: + +- URL's protocol is in `el-get-secure-protocols' + +- URL starts with 'file:///' (without hostname), so it points to the + local file + +- URL starts with username, i.e. 'username@example.com', also known as + SCP-like syntax + +- PACKAGE definition has a non-empty :checksum" + (let* ((checksum (plist-get (el-get-package-def PACKAGE) :checksum)) (checksum-empty (or (not (stringp checksum)) (if (fboundp 'string-blank-p) (string-blank-p checksum) (string-match-p "\\`[ \t\n\r]*\\'" checksum))))) (when (and (not el-get-allow-insecure) - (not (string-match "^file:///" url)) - (not (string-match "^https://" url)) - (not (string-match "^[-_\.A-Za-z0-9]+@" url)) - (not (string-match "^sftp://" url)) - (not (string-match "^bzr\\+ssh://" url)) - (not (string-match "^git\\+ssh://" url)) - (not (string-match "^ssh" url))) + (not (string-match "\\`file:///" URL)) + (not (car (member 0 (mapcar (lambda (secure-proto) + (let ((proto-rx (concat "\\`" (regexp-quote secure-proto) "://"))) + (string-match-p proto-rx URL))) el-get-secure-protocols)))) + (not (string-match "\\`[-_\.A-Za-z0-9]+@" URL))) ;; With not empty :checksum, we can rely on `el-get-post-install' calling ;; `el-get-verify-checksum' for security. (unless (not checksum-empty) - (error (concat "Attempting to install package " - (el-get-as-string package) - " from insecure URL " url + (error (concat "Attempting to install PACKAGE " + (el-get-as-string PACKAGE) + " from insecure URL " URL " without `el-get-allow-insecure'.")))))) (require 'el-get-apt-get) diff --git a/test/el-get-tests.el b/test/el-get-tests.el index 1a295eb5..5866c12d 100644 --- a/test/el-get-tests.el +++ b/test/el-get-tests.el @@ -129,7 +129,13 @@ Following variables are bound to temporal values: (defconst insecure-urls '("http://example.com" "ftp://example.com" "file://example.com/home/user" - ":pserver:anonymous@example.com")) + ":pserver:anonymous@example.com" + " +https://example.com" + " +file:///home/user" + " +John.Doe-123_@example.com")) (ert-deftest el-get-insecure-check-insecure () "Insecure URL for a package without :checksum"