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 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..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,11 +19,12 @@ 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; 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 +43,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; @@ -102,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) { - } } } @@ -400,40 +386,39 @@ 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; + checkFileName(fileId); + 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."); + + "file does not exist or is not readable."); } - 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); - } + return filePath.toFile(); + } - return file; + /** + * 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."); + } } } 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"); 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; 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()))); 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(); } 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; 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(); 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 */