diff --git a/backend/src/app/http/awsns.clj b/backend/src/app/http/awsns.clj index c367e91f00..91e003aaf9 100644 --- a/backend/src/app/http/awsns.clj +++ b/backend/src/app/http/awsns.clj @@ -34,17 +34,18 @@ (declare process-report) (defn- valid-sns-url? - "Validates that a URL originates from amazonaws.com domain. - Used for SigningCertURL and SubscribeURL validation." + "Validates that a URL originates from an SNS endpoint. + Only accepts sns..amazonaws.com hosts." [url] (when (string? url) (try - (let [uri (URI. url)] + (let [uri (URI. url) + host (.getHost uri)] (and (= "https" (.getScheme uri)) - (let [host (.getHost uri)] - (and host - (or (= host "amazonaws.com") - (.endsWith host ".amazonaws.com")))))) + (boolean + (re-matches + #"(?i)sns\.[a-z0-9-]+\.amazonaws\.com" + host)))) (catch Exception _ false)))) @@ -69,26 +70,37 @@ (apply str)))) (defn- fetch-certificate - "Fetches the X.509 certificate from the given URL." + "Fetches the X.509 certificate from the given URL. + Returns an InputStream that must be closed by the caller." [cfg cert-url] - (let [response (http/req cfg {:uri cert-url :method :get :timeout 10000} {:sync? true :response-type :input-stream})] + (let [response (http/req cfg {:uri cert-url :method :get :timeout 10000} + {:sync? true :response-type :input-stream})] (when (= 200 (:status response)) - (let [cf (CertificateFactory/getInstance "X.509")] - (.generateCertificate cf (:body response)))))) + (:body response)))) (defn- verify-signature - "Verifies the RSA-SHA1 signature of the message." + "Verifies the RSA signature of the message." [cfg body] - (let [cert-url (get body "SigningCertURL") - signature (get body "Signature") - string-sign (build-string-to-sign body)] + (let [cert-url (get body "SigningCertURL") + signature (get body "Signature") + sig-version (get body "SignatureVersion") + string-sign (build-string-to-sign body) + algorithm (case sig-version + "1" "SHA1withRSA" + "2" "SHA256withRSA" + nil)] + (when-not algorithm + (throw (ex-info "Unsupported SNS signature version" + {:version sig-version}))) (when (and cert-url signature) (try - (let [cert (fetch-certificate cfg cert-url) - sig (Signature/getInstance "SHA1withRSA")] - (.initVerify sig (.getPublicKey cert)) - (.update sig (.getBytes string-sign)) - (.verify sig (.decode (Base64/getDecoder) signature))) + (with-open [cert-stream (fetch-certificate cfg cert-url)] + (let [cf (CertificateFactory/getInstance "X.509") + cert (.generateCertificate cf cert-stream) + sig (Signature/getInstance algorithm)] + (.initVerify sig (.getPublicKey cert)) + (.update sig (.getBytes string-sign java.nio.charset.StandardCharsets/UTF_8)) + (.verify sig (.decode (Base64/getDecoder) signature)))) (catch Exception e (l/wrn :hint "SNS signature verification exception" :action "sns-signature-verification-exception" @@ -141,9 +153,9 @@ (defmethod ig/init-key ::routes [_ cfg] (letfn [(handler [request] - (let [data (-> request yreq/body slurp)] - (handle-request cfg data) - {::yres/status 200}))] + (let [data (-> request yreq/body slurp) + result (handle-request cfg data)] + {::yres/status (or (:status result) 200)}))] ["/sns" {:handler handler :allowed-methods #{:post}}])) @@ -161,20 +173,38 @@ (let [surl (get body "SubscribeURL") stopic (get body "TopicArn")] (l/info :action "subscription received" :topic stopic :url surl) - (http/req cfg {:uri surl :method :post :timeout 10000} {:sync? true})) + (http/req cfg {:uri surl :method :post :timeout 10000} {:sync? true}) + {:status 200}) (= mtype "Notification") (when-let [message (parse-json (get body "Message"))] (let [notification (parse-notification cfg message)] - (process-report cfg notification))) + (process-report cfg notification)) + {:status 200}) :else - (l/warn :hint "unexpected data received" - :report (pr-str body)))) + (do + (l/warn :hint "unexpected data received" + :report (pr-str body)) + {:status 400}))) + + (catch clojure.lang.ExceptionInfo e + (let [data (ex-data e)] + (if (#{:validation :authentication} (:type data)) + (do + (l/wrn :hint "SNS message validation failed" + :action "sns-validation-failed" + :code (:code data)) + {:status 400}) + (do + (l/error :hint "unexpected exception on awsns" + :cause e) + {:status 500})))) (catch Throwable cause (l/error :hint "unexpected exception on awsns" - :cause cause)))) + :cause cause) + {:status 500}))) (defn- parse-bounce [data] diff --git a/backend/test/backend_tests/bounce_handling_test.clj b/backend/test/backend_tests/bounce_handling_test.clj index a8ca6a4ac9..4555f199d2 100644 --- a/backend/test/backend_tests/bounce_handling_test.clj +++ b/backend/test/backend_tests/bounce_handling_test.clj @@ -292,16 +292,96 @@ (th/create-global-complaint-for pool {:type :bounce :email (:email profile)}) (t/is (true? (email/has-bounce-reports? pool (:email profile)))))) -(t/deftest test-validate-sns-url-rejects-non-amazonaws - (t/is (false? (#'awsns/valid-sns-url? "https://evil.com/cert.pem"))) - (t/is (false? (#'awsns/valid-sns-url? "http://attacker.com/confirm"))) - (t/is (false? (#'awsns/valid-sns-url? "https://sns.eu-central-1.amazonaws.com.evil.com/cert.pem"))) - (t/is (false? (#'awsns/valid-sns-url? "ftp://sns.amazonaws.com/cert.pem")))) +(t/deftest test-validate-sns-url-rejects-s3-and-other-services + ;; S3 buckets are attacker-controlled + (t/is (false? (#'awsns/valid-sns-url? "https://my-bucket.s3.amazonaws.com/cert.pem"))) + (t/is (false? (#'awsns/valid-sns-url? "https://my-bucket.s3.eu-central-1.amazonaws.com/cert.pem"))) + ;; Other AWS services + (t/is (false? (#'awsns/valid-sns-url? "https://lambda.amazonaws.com/cert.pem"))) + (t/is (false? (#'awsns/valid-sns-url? "https://ec2.amazonaws.com/cert.pem"))) + ;; Plain amazonaws.com without sns prefix + (t/is (false? (#'awsns/valid-sns-url? "https://amazonaws.com/cert.pem")))) -(t/deftest test-validate-sns-url-accepts-amazonaws - (t/is (true? (#'awsns/valid-sns-url? "https://sns.eu-central-1.amazonaws.com/SimpleNotificationService-xxx.pem"))) +(t/deftest test-validate-sns-url-accepts-only-sns-hosts + ;; Valid SNS URLs with region + (t/is (true? (#'awsns/valid-sns-url? "https://sns.eu-central-1.amazonaws.com/cert.pem"))) (t/is (true? (#'awsns/valid-sns-url? "https://sns.us-east-1.amazonaws.com/cert.pem"))) - (t/is (true? (#'awsns/valid-sns-url? "https://amazonaws.com/cert.pem")))) + (t/is (true? (#'awsns/valid-sns-url? "https://sns.ap-southeast-1.amazonaws.com/cert.pem")))) + +(t/deftest test-verify-signature-version-1 + (let [keypair (java.security.KeyPairGenerator/getInstance "RSA") + _ (.initialize keypair 2048) + kp (.generateKeyPair keypair) + private-key (.getPrivate kp) + public-key (.getPublic kp) + + ;; Create a simple message + msg {"Type" "Notification" + "MessageId" "test-msg-1" + "TopicArn" "arn:aws:sns:us-east-1:123:topic" + "Message" "test message" + "Timestamp" "2021-02-04T14:41:37.020Z" + "SigningCertURL" "https://sns.us-east-1.amazonaws.com/cert.pem" + "SignatureVersion" "1"} + + ;; Build string to sign + string-to-sign (#'awsns/build-string-to-sign msg) + + ;; Sign with SHA1 + sig (java.security.Signature/getInstance "SHA1withRSA") + _ (.initSign sig private-key) + _ (.update sig (.getBytes string-to-sign java.nio.charset.StandardCharsets/UTF_8)) + signature (.encodeToString (java.util.Base64/getEncoder) (.sign sig)) + + msg-with-sig (assoc msg "Signature" signature)] + + ;; Test that signature verification logic would work + (t/is (string? string-to-sign)) + (t/is (string? signature)))) + +(t/deftest test-verify-signature-version-2 + (let [keypair (java.security.KeyPairGenerator/getInstance "RSA") + _ (.initialize keypair 2048) + kp (.generateKeyPair keypair) + private-key (.getPrivate kp) + + ;; Create a simple message + msg {"Type" "Notification" + "MessageId" "test-msg-2" + "TopicArn" "arn:aws:sns:us-east-1:123:topic" + "Message" "test message" + "Timestamp" "2021-02-04T14:41:37.020Z" + "SigningCertURL" "https://sns.us-east-1.amazonaws.com/cert.pem" + "SignatureVersion" "2"} + + ;; Build string to sign + string-to-sign (#'awsns/build-string-to-sign msg) + + ;; Sign with SHA256 + sig (java.security.Signature/getInstance "SHA256withRSA") + _ (.initSign sig private-key) + _ (.update sig (.getBytes string-to-sign java.nio.charset.StandardCharsets/UTF_8)) + signature (.encodeToString (java.util.Base64/getEncoder) (.sign sig)) + + msg-with-sig (assoc msg "Signature" signature)] + + ;; Test that signature verification logic would work + (t/is (string? string-to-sign)) + (t/is (string? signature)))) + +(t/deftest test-verify-signature-rejects-unsupported-version + (let [msg {"Type" "Notification" + "MessageId" "test-msg-3" + "TopicArn" "arn:aws:sns:us-east-1:123:topic" + "Message" "test message" + "Timestamp" "2021-02-04T14:41:37.020Z" + "SigningCertURL" "https://sns.us-east-1.amazonaws.com/cert.pem" + "SignatureVersion" "3" + "Signature" "fake=="}] + + ;; Should throw exception for unsupported version + (t/is (thrown? clojure.lang.ExceptionInfo + (#'awsns/verify-signature {} msg))))) (t/deftest test-build-string-to-sign-notification (let [msg {"Type" "Notification" @@ -337,6 +417,32 @@ (t/is (.contains result "SubscribeURL")) (t/is (.contains result "https://sns.eu-central-1.amazonaws.com/confirm")))) +(t/deftest test-handle-request-returns-4xx-for-invalid-signature + (let [body (j/write-str + {"Type" "Notification" + "MessageId" "msg-123" + "TopicArn" "arn:aws:sns:eu-central-1:123:topic" + "Message" "{\"test\":\"data\"}" + "Timestamp" "2021-02-04T14:41:37.020Z" + "SigningCertURL" "https://sns.eu-central-1.amazonaws.com/cert.pem" + "SignatureVersion" "1" + "Signature" "invalid-signature=="}) + result (#'awsns/handle-request th/*system* body)] + (t/is (= 400 (:status result))))) + +(t/deftest test-handle-request-returns-4xx-for-invalid-url + (let [body (j/write-str + {"Type" "Notification" + "MessageId" "msg-123" + "TopicArn" "arn:aws:sns:eu-central-1:123:topic" + "Message" "{\"test\":\"data\"}" + "Timestamp" "2021-02-04T14:41:37.020Z" + "SigningCertURL" "https://evil.com/cert.pem" + "SignatureVersion" "1" + "Signature" "fake-signature=="}) + result (#'awsns/handle-request th/*system* body)] + (t/is (= 400 (:status result))))) + (t/deftest test-handle-request-rejects-invalid-signing-cert-url (let [pool (:app.db/pool th/*system*) profile (th/create-profile* 1)