From bf2652fd6c6aefe58e1ec3bbf8fca963f82af38f Mon Sep 17 00:00:00 2001 From: Michael Bien Date: Sun, 22 Aug 2021 03:44:19 +0200 Subject: [PATCH 01/10] RememberMeService should use a better hash function. --- .../ui/core/security/RollerRememberMeServices.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/core/security/RollerRememberMeServices.java b/app/src/main/java/org/apache/roller/weblogger/ui/core/security/RollerRememberMeServices.java index af1afc2b43..2566a4308d 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/core/security/RollerRememberMeServices.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/core/security/RollerRememberMeServices.java @@ -31,8 +31,8 @@ public class RollerRememberMeServices extends TokenBasedRememberMeServices { - private static final Log log = LogFactory.getLog(RollerRememberMeServices.class); + private static final Log log = LogFactory.getLog(RollerRememberMeServices.class); public RollerRememberMeServices(UserDetailsService userDetailsService) { @@ -51,7 +51,7 @@ public RollerRememberMeServices(UserDetailsService userDetailsService) { /** * Calculates the digital signature to be put in the cookie. Default value is - * MD5 ("username:tokenExpiryTime:password:key") + * SHA-512 ("username:tokenExpiryTime:password:key") * * If LDAP is enabled then a configurable dummy password is used in the calculation. */ @@ -70,9 +70,9 @@ protected String makeTokenSignature(long tokenExpiryTime, String username, Strin String data = username + ":" + tokenExpiryTime + ":" + password + ":" + getKey(); MessageDigest digest; try { - digest = MessageDigest.getInstance("MD5"); + digest = MessageDigest.getInstance("SHA-512"); } catch (NoSuchAlgorithmException e) { - throw new IllegalStateException("No MD5 algorithm available!"); + throw new IllegalStateException("Required by Spec.", e); } return new String(Hex.encode(digest.digest(data.getBytes()))); From e6160c3ece8e7c920e6e59c2c26d523c7165fa45 Mon Sep 17 00:00:00 2001 From: Michael Bien Date: Mon, 23 Aug 2021 03:11:31 +0200 Subject: [PATCH 02/10] Context URL validation. --- .../weblogger/ui/core/filters/InitFilter.java | 42 +++++++++++-------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/InitFilter.java b/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/InitFilter.java index 7ab9fa094f..554ccc6beb 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/InitFilter.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/core/filters/InitFilter.java @@ -30,6 +30,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.apache.commons.validator.routines.UrlValidator; import org.apache.roller.weblogger.config.WebloggerRuntimeConfig; /** @@ -41,7 +42,7 @@ */ public class InitFilter implements Filter { - private static Log log = LogFactory.getLog(InitFilter.class); + private static final Log log = LogFactory.getLog(InitFilter.class); private boolean initialized = false; @@ -53,22 +54,29 @@ public void doFilter(ServletRequest req, ServletResponse res, // first request, lets do our initialization HttpServletRequest request = (HttpServletRequest) req; - // HttpServletResponse response = (HttpServletResponse) res; - - // determine absolute and relative url paths to the app - String relPath = request.getContextPath(); - String absPath = this.getAbsoluteUrl(request); - - // set them in our config - WebloggerRuntimeConfig.setAbsoluteContextURL(absPath); - WebloggerRuntimeConfig.setRelativeContextURL(relPath); - - if (log.isDebugEnabled()) { - log.debug("relPath = " + relPath); - log.debug("absPath = " + absPath); + + UrlValidator validator = new UrlValidator( + new String[]{"http", "https"}, + UrlValidator.ALLOW_LOCAL_URLS); // for integration tests + + if(validator.isValid(request.getRequestURL().toString())) { + + // determine absolute and relative url paths to the app + String relPath = request.getContextPath(); + String absPath = this.getAbsoluteUrl(request); + + // set them in our config + WebloggerRuntimeConfig.setAbsoluteContextURL(absPath); + WebloggerRuntimeConfig.setRelativeContextURL(relPath); + + if (log.isDebugEnabled()) { + log.debug("relPath = " + relPath); + log.debug("absPath = " + absPath); + } + + this.initialized = true; } - this.initialized = true; } chain.doFilter(req, res); @@ -90,9 +98,9 @@ public void destroy() { protected static String getAbsoluteUrl(boolean secure, String serverName, String contextPath, String requestURI, String requestURL){ - String url = null; + String url; - String fullUrl = null; + String fullUrl; if (!secure) { fullUrl = requestURL; From 0a18cc999ea9cc4532a796e027ca80f874cf3911 Mon Sep 17 00:00:00 2001 From: Michael Bien Date: Mon, 23 Aug 2021 06:43:07 +0200 Subject: [PATCH 03/10] TagDataServlet: Escape URIs for XML output to make CodeQL happy. This is technically not needed, but CodeQL thinks those variables are client provided Strings, since one code path leads to the InitFilter. We do it anyway to fix 3 alerts + its trivial. --- .../weblogger/webservices/tagdata/TagDataServlet.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/webservices/tagdata/TagDataServlet.java b/app/src/main/java/org/apache/roller/weblogger/webservices/tagdata/TagDataServlet.java index 6ddb591576..e239839ebb 100644 --- a/app/src/main/java/org/apache/roller/weblogger/webservices/tagdata/TagDataServlet.java +++ b/app/src/main/java/org/apache/roller/weblogger/webservices/tagdata/TagDataServlet.java @@ -186,7 +186,7 @@ public void doGet( 0, true); int frequency = stat.getCount(); pw.print(""); + pw.println("tagdata:href=\"" + StringEscapeUtils.escapeXml10(viewURI) + "\" />"); if (count++ > MAX) { break; } @@ -194,12 +194,12 @@ public void doGet( if (tags.size() > MAX) { // get next URI, if site-wide then don't specify weblog String nextURI = urlstrat.getWeblogTagsJsonURL(weblog, true, page + 1); - pw.println(""); + pw.println(""); } if (page > 0) { // get prev URI, if site-wide then don't specify weblog String prevURI = urlstrat.getWeblogTagsJsonURL(weblog, true, page - 1); - pw.println(""); + pw.println(""); } pw.println(""); response.flushBuffer(); From 8ea6e9def09e0914550e52d71ada2faf4996fa6a Mon Sep 17 00:00:00 2001 From: Michael Bien Date: Tue, 24 Aug 2021 06:05:43 +0200 Subject: [PATCH 04/10] FileContentManagerImpl: Validate Path before creating a File. CodeQL doesn't understand validation which is happening *after* Files or Paths are created. Rewriting the method a little bit solves that + its now using Path instead of File. --- .../business/FileContentManagerImpl.java | 45 +++++++++---------- 1 file changed, 20 insertions(+), 25 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/business/FileContentManagerImpl.java b/app/src/main/java/org/apache/roller/weblogger/business/FileContentManagerImpl.java index cd8553daaa..0b99268334 100644 --- a/app/src/main/java/org/apache/roller/weblogger/business/FileContentManagerImpl.java +++ b/app/src/main/java/org/apache/roller/weblogger/business/FileContentManagerImpl.java @@ -24,6 +24,8 @@ import java.io.InputStream; import java.io.OutputStream; import java.math.BigDecimal; +import java.nio.file.Files; +import java.nio.file.Path; import org.apache.commons.lang3.StringUtils; import org.apache.commons.logging.Log; @@ -42,7 +44,7 @@ */ public class FileContentManagerImpl implements FileContentManager { - private static Log log = LogFactory.getLog(FileContentManagerImpl.class); + private static final Log log = LogFactory.getLog(FileContentManagerImpl.class); private String storageDir = null; @@ -400,40 +402,33 @@ private File getRealFile(Weblog weblog, String fileId) throws FileNotFoundException, FilePathException { // make sure uploads area exists for this weblog - File weblogDir = new File(this.storageDir + weblog.getHandle()); - if (!weblogDir.exists()) { - weblogDir.mkdirs(); + Path weblogDir = Path.of(this.storageDir, weblog.getHandle()); + if (!Files.exists(weblogDir)) { + try { + Files.createDirectories(weblogDir); + } catch (IOException ex) { + throw new FilePathException("Can't create storage dir [" + weblogDir + "]", ex); + } } // now form the absolute path - String filePath = weblogDir.getAbsolutePath(); + Path filePath = weblogDir.toAbsolutePath(); if (fileId != null) { - filePath += File.separator + fileId; + // make sure someone isn't trying to sneek outside the uploads dir + if(fileId.contains("..")) { + throw new FilePathException("Invalid file name [" + fileId + "], " + + "trying to get outside uploads dir."); + } + filePath = filePath.resolve(fileId); } // make sure path exists and is readable - File file = new File(filePath); - if (!file.exists()) { + if (!Files.isReadable(filePath)) { throw new FileNotFoundException("Invalid path [" + filePath + "], " - + "file does not exist."); - } else if (!file.canRead()) { - throw new FilePathException("Invalid path [" + filePath + "], " - + "cannot read from path."); - } - - try { - // make sure someone isn't trying to sneek outside the uploads dir - if (!file.getCanonicalPath().startsWith( - weblogDir.getCanonicalPath())) { - throw new FilePathException("Invalid path " + filePath + "], " - + "trying to get outside uploads dir."); - } - } catch (IOException ex) { - // rethrow as FilePathException - throw new FilePathException(ex); + + "file does not exist or is not readable."); } - return file; + return filePath.toFile(); } } From b438911df6f72e7ec6b8e1bec1a9f7209eb72db3 Mon Sep 17 00:00:00 2001 From: Michael Bien Date: Tue, 24 Aug 2021 21:51:37 +0200 Subject: [PATCH 05/10] FileContentManagerImpl: Validate filename in saveFileContent() + use stream transferTo() shortcut. --- .../business/FileContentManagerImpl.java | 46 ++++++++----------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/business/FileContentManagerImpl.java b/app/src/main/java/org/apache/roller/weblogger/business/FileContentManagerImpl.java index 0b99268334..3df3902edf 100644 --- a/app/src/main/java/org/apache/roller/weblogger/business/FileContentManagerImpl.java +++ b/app/src/main/java/org/apache/roller/weblogger/business/FileContentManagerImpl.java @@ -19,7 +19,6 @@ package org.apache.roller.weblogger.business; import java.io.File; -import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -104,34 +103,19 @@ public FileContent getFileContent(Weblog weblog, String fileId) public void saveFileContent(Weblog weblog, String fileId, InputStream is) throws FileNotFoundException, FilePathException, FileIOException { + checkFileName(fileId); + // make sure uploads area exists for this weblog File dirPath = this.getRealFile(weblog, null); // create File that we are about to save - File saveFile = new File(dirPath.getAbsolutePath() + File.separator - + fileId); + Path saveFile = Path.of(dirPath.getAbsolutePath(), fileId); - byte[] buffer = new byte[RollerConstants.EIGHT_KB_IN_BYTES]; - int bytesRead; - OutputStream bos = null; - try { - bos = new FileOutputStream(saveFile); - while ((bytesRead = is.read(buffer, 0, - RollerConstants.EIGHT_KB_IN_BYTES)) != -1) { - bos.write(buffer, 0, bytesRead); - } - log.debug("The file has been written to [" - + saveFile.getAbsolutePath() + "]"); - } catch (Exception e) { + try (OutputStream os = Files.newOutputStream(saveFile)) { + is.transferTo(os); + log.debug("The file has been written to ["+saveFile+"]"); + } catch (IOException e) { throw new FileIOException("ERROR uploading file", e); - } finally { - try { - if (bos != null) { - bos.flush(); - bos.close(); - } - } catch (Exception ignored) { - } } } @@ -414,11 +398,7 @@ private File getRealFile(Weblog weblog, String fileId) // now form the absolute path Path filePath = weblogDir.toAbsolutePath(); if (fileId != null) { - // make sure someone isn't trying to sneek outside the uploads dir - if(fileId.contains("..")) { - throw new FilePathException("Invalid file name [" + fileId + "], " - + "trying to get outside uploads dir."); - } + checkFileName(fileId); filePath = filePath.resolve(fileId); } @@ -431,4 +411,14 @@ private File getRealFile(Weblog weblog, String fileId) return filePath.toFile(); } + /** + * Make sure someone isn't trying to sneak outside the uploads dir. + */ + private static void checkFileName(String fileId) throws FilePathException { + if(fileId.contains("..")) { + throw new FilePathException("Invalid file name [" + fileId + "], " + + "trying to get outside uploads dir."); + } + } + } From ea8abe1a8fe1996b0a8c1218b8543e2f09fd1677 Mon Sep 17 00:00:00 2001 From: Michael Bien Date: Tue, 24 Aug 2021 22:15:21 +0200 Subject: [PATCH 06/10] FolderEdit: HTTP response splitting defense. --- .../roller/weblogger/ui/struts2/editor/FolderEdit.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/FolderEdit.java b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/FolderEdit.java index 91dc0aed51..94de22d1fc 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/FolderEdit.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/FolderEdit.java @@ -40,7 +40,7 @@ // TODO: make this work @AllowedMethods({"execute","save"}) public class FolderEdit extends UIAction implements ServletResponseAware { - private static Log log = LogFactory.getLog(FolderEdit.class); + private static final Log log = LogFactory.getLog(FolderEdit.class); // bean for managing form data private FolderBean bean = new FolderBean(); @@ -127,7 +127,10 @@ public String save() { addMessage("folderForm.updated"); } - httpServletResponse.addHeader("folderId", folderId ); + // HTTP response splitting defense + String sanetizedFolderID = folderId.replace("\n", "").replace("\r", ""); + + httpServletResponse.addHeader("folderId", sanetizedFolderID); return SUCCESS; From b443ee4f4510f67781aab35e3c6dbefefc605060 Mon Sep 17 00:00:00 2001 From: Michael Bien Date: Wed, 25 Aug 2021 00:29:00 +0200 Subject: [PATCH 07/10] WeblogRequestMapper: Use already validated weblog handle for redirect logic. --- .../roller/weblogger/ui/rendering/WeblogRequestMapper.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/ui/rendering/WeblogRequestMapper.java b/app/src/main/java/org/apache/roller/weblogger/ui/rendering/WeblogRequestMapper.java index 92b78a2c83..584ee2813c 100644 --- a/app/src/main/java/org/apache/roller/weblogger/ui/rendering/WeblogRequestMapper.java +++ b/app/src/main/java/org/apache/roller/weblogger/ui/rendering/WeblogRequestMapper.java @@ -46,7 +46,7 @@ */ public class WeblogRequestMapper implements RequestMapper { - private static Log log = LogFactory.getLog(WeblogRequestMapper.class); + private static final Log log = LogFactory.getLog(WeblogRequestMapper.class); private static final String PAGE_SERVLET = "/roller-ui/rendering/page"; private static final String FEED_SERVLET = "/roller-ui/rendering/feed"; @@ -199,7 +199,7 @@ public boolean handleRequest(HttpServletRequest request, HttpServletResponse res // this means someone referred to a weblog index page with the // shortest form of url / or // and we need // to do a redirect to // or /// - String redirectUrl = request.getRequestURI() + "/"; + String redirectUrl = "/" + weblogHandle + "/"; if(request.getQueryString() != null) { redirectUrl += "?"+request.getQueryString(); } From 96810ea0e50a353fa6ba37f14f9d2747cf249541 Mon Sep 17 00:00:00 2001 From: Michael Bien Date: Wed, 25 Aug 2021 01:01:29 +0200 Subject: [PATCH 08/10] close the right stream (getter would return a new stream). --- .../roller/weblogger/business/themes/ThemeManagerImpl.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeManagerImpl.java b/app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeManagerImpl.java index 86032632f6..13467171ce 100644 --- a/app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeManagerImpl.java +++ b/app/src/main/java/org/apache/roller/weblogger/business/themes/ThemeManagerImpl.java @@ -65,7 +65,7 @@ @com.google.inject.Singleton public class ThemeManagerImpl implements ThemeManager { - static FileTypeMap map = null; + private static final FileTypeMap map; static { // TODO: figure out why PNG is missing from Java MIME types map = FileTypeMap.getDefaultFileTypeMap(); @@ -77,7 +77,7 @@ public class ThemeManagerImpl implements ThemeManager { } } - private static Log log = LogFactory.getLog(ThemeManagerImpl.class); + private static final Log log = LogFactory.getLog(ThemeManagerImpl.class); private final Weblogger roller; // directory where themes are kept private String themeDir = null; @@ -354,7 +354,7 @@ public void importTheme(Weblog weblog, SharedTheme theme, boolean skipStylesheet RollerMessages errors = new RollerMessages(); fileMgr.createThemeMediaFile(weblog, mf, errors); try { - resource.getInputStream().close(); + is.close(); } catch (IOException ex) { errors.addError("error.closingStream"); log.debug("ERROR closing inputstream"); From 188e201ee02769686991019a8a521a1ca3d3b96f Mon Sep 17 00:00:00 2001 From: Michael Bien Date: Wed, 25 Aug 2021 04:11:39 +0200 Subject: [PATCH 09/10] set cookie "secure" and "SameSite" flags by default. --- app/src/main/webapp/theme/scripts/roller.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/src/main/webapp/theme/scripts/roller.js b/app/src/main/webapp/theme/scripts/roller.js index 1685b7642f..f703a622d7 100644 --- a/app/src/main/webapp/theme/scripts/roller.js +++ b/app/src/main/webapp/theme/scripts/roller.js @@ -16,11 +16,12 @@ * directory of this distribution. */ /* This function is used to set cookies */ -function setCookie(name,value,expires,path,domain,secure) { +function setCookie(name, value, expires, path, domain, secure=true, sameSite=true) { document.cookie = name + "=" + escape (value) + ((expires) ? "; expires=" + expires.toGMTString() : "") + ((path) ? "; path=" + path : "") + - ((domain) ? "; domain=" + domain : "") + ((secure) ? "; secure" : ""); + ((domain) ? "; domain=" + domain : "") + ((secure) ? "; secure" : "") + + ((sameSite) ? "; SameSite=Strict" : ""); } /* This function is used to get cookies */ From 75ba7959557453bf9d59495473bbab33f5775caf Mon Sep 17 00:00:00 2001 From: Michael Bien Date: Fri, 27 Aug 2021 05:38:38 +0200 Subject: [PATCH 10/10] CodeQL: don't scan JS files three times. this requires unfortunately another config file since path settings can't be set in the workflow config. see https://github.com/github/codeql-action/issues/283 --- .github/codeql/codeql-config.yml | 14 ++++++++++++++ .github/workflows/codeql-analysis.yml | 10 +--------- 2 files changed, 15 insertions(+), 9 deletions(-) create mode 100644 .github/codeql/codeql-config.yml diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml new file mode 100644 index 0000000000..7fa5e23b81 --- /dev/null +++ b/.github/codeql/codeql-config.yml @@ -0,0 +1,14 @@ +name: "Roller CodeQL config" + +# paths-ignore only influences interpreted languages according to the doc +# don't scan JS files three times: +# - ignore test folder and source folder +# - target is kept to only scan what is deployed +paths-ignore: + - app/target/test-classes + - app/src + +# If you wish to specify custom queries, you can do so here or in a config file. +# By default, queries listed here will override any specified in a config file. +# Prefix the list here with "+" to use these queries and those in the config file. +# queries: ./path/to/local/query, your-org/your-repo/queries@main diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 4a7f7ba0e7..43f56cbc3a 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -4,11 +4,6 @@ # You may wish to alter this file to override the set of languages analyzed, # or to provide custom queries or build logic. # -# ******** NOTE ******** -# We have attempted to detect the languages in your repository. Please check -# the `language` matrix defined below to confirm you have the correct set of -# supported CodeQL languages. -# name: "CodeQL" on: @@ -45,10 +40,7 @@ jobs: uses: github/codeql-action/init@v1 with: languages: ${{ matrix.language }} - # If you wish to specify custom queries, you can do so here or in a config file. - # By default, queries listed here will override any specified in a config file. - # Prefix the list here with "+" to use these queries and those in the config file. - # queries: ./path/to/local/query, your-org/your-repo/queries@main + config-file: ./.github/codeql/codeql-config.yml - name: Build with Maven run: mvn -DskipTests=true -V -ntp install