From a9a7f17910471482640132a5c22cb037bd12a7cc Mon Sep 17 00:00:00 2001 From: Latchezar Tzvetkoff Date: Fri, 15 Jul 2016 18:01:41 +0300 Subject: [PATCH 1/7] Allow modules to override CSRF protection. Useful for Web APIs and all other kinds of things. API changes: - Added public CHTTPSock::GetURI() method - Added public CModule::ValidateWebRequestCSRFCheck() method - Made CWebSock::GetCSRFCheck() method public so it can be accessed from CModule - Added public CWebSock::ValidateCSRFCheck() method Other changes: - Added a Sample Web API module (modules/samplewebapi.cpp) and a simple web form with no CSRF check. Implements feature request #1180. --- include/znc/HTTPSock.h | 1 + include/znc/Modules.h | 6 +++ include/znc/WebModules.h | 4 +- modules/data/samplewebapi/tmpl/index.tmpl | 21 +++++++++ modules/samplewebapi.cpp | 56 +++++++++++++++++++++++ src/HTTPSock.cpp | 2 + src/Modules.cpp | 4 ++ src/WebModules.cpp | 20 +++++++- 8 files changed, 111 insertions(+), 3 deletions(-) create mode 100644 modules/data/samplewebapi/tmpl/index.tmpl create mode 100644 modules/samplewebapi.cpp diff --git a/include/znc/HTTPSock.h b/include/znc/HTTPSock.h index 5ec4453c..195b8b6a 100644 --- a/include/znc/HTTPSock.h +++ b/include/znc/HTTPSock.h @@ -83,6 +83,7 @@ class CHTTPSock : public CSocket { const CString& GetPass() const; const CString& GetParamString() const; const CString& GetContentType() const; + const CString& GetURI() const; const CString& GetURIPrefix() const; bool IsPost() const; // !Getters diff --git a/include/znc/Modules.h b/include/znc/Modules.h index 4eaab848..063d0040 100644 --- a/include/znc/Modules.h +++ b/include/znc/Modules.h @@ -477,6 +477,12 @@ class CModule { */ virtual bool OnWebRequest(CWebSock& WebSock, const CString& sPageName, CTemplate& Tmpl); + /** If ValidateWebRequestCSRFCheck returned false, a CSRF error will be printed. + * @param WebSock The active request. + * @param sPageName The name of the page that has been requested. + * @return You MUST return true if the CSRF token is valid. + */ + virtual bool ValidateWebRequestCSRFCheck(CWebSock& WebSock, const CString& sPageName); /** Registers a sub page for the sidebar. * @param spSubPage The SubPage instance. */ diff --git a/include/znc/WebModules.h b/include/znc/WebModules.h index 576cfffc..f2b36b41 100644 --- a/include/znc/WebModules.h +++ b/include/znc/WebModules.h @@ -178,6 +178,9 @@ class CWebSock : public CHTTPSock { static void FinishUserSessions(const CUser& User); + CString GetCSRFCheck(); + bool ValidateCSRFCheck(const CString& sURI); + protected: using CHTTPSock::PrintErrorPage; @@ -186,7 +189,6 @@ class CWebSock : public CHTTPSock { VCString GetDirs(CModule* pModule, bool bIsTemplate); void SetPaths(CModule* pModule, bool bIsTemplate = false); void SetVars(); - CString GetCSRFCheck(); private: EPageReqResult OnPageRequestInternal(const CString& sURI, diff --git a/modules/data/samplewebapi/tmpl/index.tmpl b/modules/data/samplewebapi/tmpl/index.tmpl new file mode 100644 index 00000000..bebddd78 --- /dev/null +++ b/modules/data/samplewebapi/tmpl/index.tmpl @@ -0,0 +1,21 @@ + + +
+
+

Sample Web API

+
+
+
+
Text:
+ +
Sample text that will be returned plain on submit/API request. +
+
+ +
+
+
+
+
+ + diff --git a/modules/samplewebapi.cpp b/modules/samplewebapi.cpp new file mode 100644 index 00000000..b26cd6a9 --- /dev/null +++ b/modules/samplewebapi.cpp @@ -0,0 +1,56 @@ +/* + * Copyright (C) 2004-2016 ZNC, see the NOTICE file for details. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +class CSampleWebAPIMod : public CModule { + public: + MODCONSTRUCTOR(CSampleWebAPIMod) {} + + ~CSampleWebAPIMod() override {} + + bool OnWebRequest(CWebSock& WebSock, const CString& sPageName, + CTemplate& Tmpl) override { + if (sPageName != "index") { + // only accept requests to index + return false; + } + + if (WebSock.IsPost()) { + // print the text we just recieved + CString text = WebSock.GetRawParam("text", true); + WebSock.PrintHeader(text.length(), "text/plain; charset=UTF-8"); + WebSock.Write(text); + WebSock.Close(Csock::CLT_AFTERWRITE); + return false; + } + + return true; + } + + bool ValidateWebRequestCSRFCheck(CWebSock& WebSock, + const CString& sPageName) override { + return true; + } +}; + +template <> +void TModInfo(CModInfo& Info) { + Info.AddType(CModInfo::UserModule); + Info.SetWikiPage("samplewebapi"); +} + +GLOBALMODULEDEFS(CSampleWebAPIMod, "Sample Web API module.") diff --git a/src/HTTPSock.cpp b/src/HTTPSock.cpp index c2958b62..07b53905 100644 --- a/src/HTTPSock.cpp +++ b/src/HTTPSock.cpp @@ -536,6 +536,8 @@ const CString& CHTTPSock::GetContentType() const { return m_sContentType; } const CString& CHTTPSock::GetParamString() const { return m_sPostData; } +const CString& CHTTPSock::GetURI() const { return m_sURI; } + const CString& CHTTPSock::GetURIPrefix() const { return m_sURIPrefix; } bool CHTTPSock::HasParam(const CString& sName, bool bPost) const { diff --git a/src/Modules.cpp b/src/Modules.cpp index f1bd5e6f..ecd8fb8d 100644 --- a/src/Modules.cpp +++ b/src/Modules.cpp @@ -594,6 +594,10 @@ bool CModule::OnWebRequest(CWebSock& WebSock, const CString& sPageName, CTemplate& Tmpl) { return false; } +bool CModule::ValidateWebRequestCSRFCheck(CWebSock& WebSock, + const CString& sPageName) { + return WebSock.ValidateCSRFCheck(WebSock.GetURI()); +} bool CModule::OnEmbeddedWebRequest(CWebSock& WebSock, const CString& sPageName, CTemplate& Tmpl) { return false; diff --git a/src/WebModules.cpp b/src/WebModules.cpp index f59f0883..2170d515 100644 --- a/src/WebModules.cpp +++ b/src/WebModules.cpp @@ -655,8 +655,8 @@ CWebSock::EPageReqResult CWebSock::OnPageRequestInternal(const CString& sURI, // 1. they obviously know the password, // 2. it's easier to automate some tasks e.g. user creation, without need to // care about cookies and csrf - if (IsPost() && !m_bBasicAuth && - GetParam("_CSRF_Check") != GetCSRFCheck() && sURI != "/login") { + if (IsPost() && !m_bBasicAuth && !sURI.StartsWith("/mods/") && + !ValidateCSRFCheck(sURI)) { DEBUG("Expected _CSRF_Check: " << GetCSRFCheck()); DEBUG("Actual _CSRF_Check: " << GetParam("_CSRF_Check")); PrintErrorPage( @@ -803,6 +803,18 @@ CWebSock::EPageReqResult CWebSock::OnPageRequestInternal(const CString& sURI, if (!pModule) return PAGE_NOTFOUND; + // Pass CSRF check to module. + if (IsPost() && !m_bBasicAuth && + !pModule->ValidateWebRequestCSRFCheck(*this, m_sPage)) { + DEBUG("Expected _CSRF_Check: " << GetCSRFCheck()); + DEBUG("Actual _CSRF_Check: " << GetParam("_CSRF_Check")); + PrintErrorPage( + 403, "Access denied", + "POST requests need to send " + "a secret token to prevent cross-site request forgery attacks."); + return PAGE_DONE; + } + m_Template["ModPath"] = pModule->GetWebPath(); m_Template["ModFilesPath"] = pModule->GetWebFilesPath(); @@ -969,6 +981,10 @@ CString CWebSock::GetCSRFCheck() { return pSession->GetId().MD5(); } +bool CWebSock::ValidateCSRFCheck(const CString& sURI) { + return sURI == "/login" || GetParam("_CSRF_Check") == GetCSRFCheck(); +} + bool CWebSock::OnLogin(const CString& sUser, const CString& sPass, bool bBasic) { DEBUG("=================== CWebSock::OnLogin(), basic auth? " From f387dc56c08f6dc1850ff77f86f41ca780bab64c Mon Sep 17 00:00:00 2001 From: lol768 Date: Thu, 29 Sep 2016 19:49:49 +0100 Subject: [PATCH 2/7] More relevant comments for CSRF behaviour --- src/WebModules.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/WebModules.cpp b/src/WebModules.cpp index 2170d515..b7065886 100644 --- a/src/WebModules.cpp +++ b/src/WebModules.cpp @@ -647,14 +647,15 @@ CWebSock::EPageReqResult CWebSock::OnPageRequestInternal(const CString& sURI, return PAGE_DONE; } - // Check that they really POSTed from one our forms by checking if they + // For pages *not provided* by modules, a CSRF check is performed which involves: + // Ensure that they really POSTed from one our forms by checking if they // know the "secret" CSRF check value. Don't do this for login since // CSRF against the login form makes no sense and the login form does a // cookies-enabled check which would break otherwise. // Don't do this, if user authenticated using http-basic auth, because: // 1. they obviously know the password, // 2. it's easier to automate some tasks e.g. user creation, without need to - // care about cookies and csrf + // care about cookies and CSRF if (IsPost() && !m_bBasicAuth && !sURI.StartsWith("/mods/") && !ValidateCSRFCheck(sURI)) { DEBUG("Expected _CSRF_Check: " << GetCSRFCheck()); @@ -804,6 +805,7 @@ CWebSock::EPageReqResult CWebSock::OnPageRequestInternal(const CString& sURI, if (!pModule) return PAGE_NOTFOUND; // Pass CSRF check to module. + // Note that the normal CSRF checks are not applied to /mods/ URLs. if (IsPost() && !m_bBasicAuth && !pModule->ValidateWebRequestCSRFCheck(*this, m_sPage)) { DEBUG("Expected _CSRF_Check: " << GetCSRFCheck()); From 0393153a6217ec50329ac32bc81da7bbe04d7ed2 Mon Sep 17 00:00:00 2001 From: lol768 Date: Thu, 29 Sep 2016 20:44:56 +0100 Subject: [PATCH 3/7] Allow anonymous access for sample mod This makes testing simpler. --- modules/samplewebapi.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/samplewebapi.cpp b/modules/samplewebapi.cpp index b26cd6a9..f5c371bf 100644 --- a/modules/samplewebapi.cpp +++ b/modules/samplewebapi.cpp @@ -41,6 +41,10 @@ class CSampleWebAPIMod : public CModule { return true; } + bool WebRequiresLogin() { + return false; + } + bool ValidateWebRequestCSRFCheck(CWebSock& WebSock, const CString& sPageName) override { return true; From 3930c5b34e45baa184f548e6bbed8f13aa769134 Mon Sep 17 00:00:00 2001 From: lol768 Date: Thu, 29 Sep 2016 20:45:19 +0100 Subject: [PATCH 4/7] Sample web API doesn't make sense as a user module --- modules/samplewebapi.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/samplewebapi.cpp b/modules/samplewebapi.cpp index f5c371bf..4518d173 100644 --- a/modules/samplewebapi.cpp +++ b/modules/samplewebapi.cpp @@ -53,7 +53,7 @@ class CSampleWebAPIMod : public CModule { template <> void TModInfo(CModInfo& Info) { - Info.AddType(CModInfo::UserModule); + Info.AddType(CModInfo::GlobalModule); Info.SetWikiPage("samplewebapi"); } From e066f896efaa28241f4fa685a63ae0c05ea42fbe Mon Sep 17 00:00:00 2001 From: lol768 Date: Thu, 29 Sep 2016 20:45:40 +0100 Subject: [PATCH 5/7] Add integration test for module CSRF overrides --- test/integration/main.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/integration/main.cpp b/test/integration/main.cpp index 23656ae3..ccf8ae33 100644 --- a/test/integration/main.cpp +++ b/test/integration/main.cpp @@ -1965,4 +1965,21 @@ TEST_F(ZNCTest, KeepNickModule) { ":Unable to obtain nick user: Nope :-P, #error"); } +TEST_F(ZNCTest, ModuleCSRFOverride) { + auto znc = Run(); + Z; + auto ircd = ConnectIRCd(); + Z; + auto client = LoginClient(); + Z; + client.Write("znc loadmod samplewebapi"); + Z; + auto request = QNetworkRequest(QUrl("http://127.0.0.1:12345/mods/global/samplewebapi/")); + auto reply = HttpPost(request, { + {"text", "ipsum"} + })->readAll().toStdString(); + Z; + EXPECT_THAT(reply, HasSubstr("ipsum")); +} + } // namespace From 9cc59b2b7847fa0eb4a46d34eb382346fe42f6a1 Mon Sep 17 00:00:00 2001 From: lol768 Date: Sat, 1 Oct 2016 20:39:14 +0100 Subject: [PATCH 6/7] Address review comment, module is already global https://github.com/lol768/znc/commit/b94c639e6a1755c33cc315aad247bbc50a83209e#commitcomment-19252797 --- modules/samplewebapi.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/samplewebapi.cpp b/modules/samplewebapi.cpp index 4518d173..6c53ce91 100644 --- a/modules/samplewebapi.cpp +++ b/modules/samplewebapi.cpp @@ -53,7 +53,6 @@ class CSampleWebAPIMod : public CModule { template <> void TModInfo(CModInfo& Info) { - Info.AddType(CModInfo::GlobalModule); Info.SetWikiPage("samplewebapi"); } From d40d87e26863e85abcd6672c811c6c2278d3bb47 Mon Sep 17 00:00:00 2001 From: lol768 Date: Sat, 1 Oct 2016 22:06:21 +0100 Subject: [PATCH 7/7] Fix race condition by using ReadUntil per review --- test/integration/main.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/test/integration/main.cpp b/test/integration/main.cpp index ccf8ae33..4a725a75 100644 --- a/test/integration/main.cpp +++ b/test/integration/main.cpp @@ -1973,6 +1973,7 @@ TEST_F(ZNCTest, ModuleCSRFOverride) { auto client = LoginClient(); Z; client.Write("znc loadmod samplewebapi"); + client.ReadUntil("Loaded module"); Z; auto request = QNetworkRequest(QUrl("http://127.0.0.1:12345/mods/global/samplewebapi/")); auto reply = HttpPost(request, {