This post is complementary to this one I asked the question about implementing DropBox's "Share link" functionality on the server side in the most "secure" way. Of course, real security involves at least password-based authentication, not this "security by obscurity" approach, but the guys from DropBox managed to implement this somehow :)
Here is the code I've written for this task. I would be grateful for any comments and criticism for this small piece of code.
package com.test;
import java.io.BufferedInputStream;
import java.io.BufferedOutputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.PrintWriter;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import javax.servlet.ServletConfig;
import javax.servlet.ServletException;
import javax.servlet.ServletOutputStream;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
/**
*
* @author Anton P. Kolosov
*/
public class ObscureSecureServlet extends HttpServlet {
private static final Pattern UUID_PATTERN = Pattern.compile("^[A-F0-9]{8}(?:-[A-F0-9]{4}){3}-[A-F0-9]{12}$", Pattern.CASE_INSENSITIVE);
private String basePath;
/**
* Initialization routines
* @param config Servlet configuration
* @throws ServletException
*/
public void init(ServletConfig config) throws ServletException {
super.init(config);
basePath = config.getInitParameter("basePath");
}
/**
* Processes requests for both HTTP <code>GET</code> and <code>POST</code> methods.
*
* @param request servlet request
* @param response servlet response
* @throws ServletException if a servlet-specific error occurs
* @throws IOException if an I/O error occurs
*/
protected void processRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
String res = request.getParameter("res");
String name = request.getParameter("name");
Matcher matcher = UUID_PATTERN.matcher(res);
if (matcher.matches()) {
// Only UUIDs are allowed for res parameter
File file = new File(basePath + "/" + res, name);
if (file.exists()) {
sendFile(file, request, response);
return;
}
}
// Can redirect to jsp if you wish...
response.setContentType("text/html;charset=UTF-8");
try (PrintWriter out = response.getWriter()) {
out.println("<!DOCTYPE html>");
out.println("<html>");
out.println("<head>");
out.println("<title>File not found</title>");
out.println("</head>");
out.println("<body>");
out.println("File for res = " + res + " and name = " + name + " was not found!");
out.println("</body>");
out.println("</html>");
}
}
private void sendFile(File file, HttpServletRequest request, HttpServletResponse response) throws IOException {
long fileSize = file.length();
response.setHeader("Content-length", Long.toString(fileSize));
response.setContentType("application/octet-stream");
response.setHeader( "Content-Disposition", "filename=\"" + file.getName() + "\"" );
ServletOutputStream out = response.getOutputStream();
BufferedOutputStream bufferedOutputStream = new BufferedOutputStream(out);
write(file, bufferedOutputStream);
bufferedOutputStream.flush();
}
/**
* Writes a document to the passed stream.
* @param bufferedOutput The method writes name to this output stream
* @throws IOException IOException
*/
public void write(File file, BufferedOutputStream bufferedOutput) throws IOException {
byte buffer[] = new byte[1024 * 4];
BufferedInputStream bufferedInput = null;
try {
FileInputStream inputStream = new FileInputStream(file);
bufferedInput = new BufferedInputStream(inputStream);
int lengthRead = 0;
int offset = 0;
while (true) {
lengthRead = bufferedInput.read(buffer, 0, buffer.length);
if (lengthRead == -1) {
break;
}
bufferedOutput.write(buffer, 0, lengthRead);
offset += lengthRead;
}
} finally {
if (bufferedInput != null) {
bufferedInput.close();
}
}
}
// <editor-fold defaultstate="collapsed" desc="HttpServlet methods. Click on the + sign on the left to edit the code.">
/**
* Handles the HTTP <code>GET</code> method.
*
* @param request servlet request
* @param response servlet response
* @throws ServletException if a servlet-specific error occurs
* @throws IOException if an I/O error occurs
*/
@Override
protected void doGet(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
processRequest(request, response);
}
/**
* Handles the HTTP <code>POST</code> method.
*
* @param request servlet request
* @param response servlet response
* @throws ServletException if a servlet-specific error occurs
* @throws IOException if an I/O error occurs
*/
@Override
protected void doPost(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
processRequest(request, response);
}
/**
* Returns a short description of the servlet.
*
* @return a String containing servlet description
*/
@Override
public String getServletInfo() {
return "Servlet for 'obscure secure' file retrieving";
}// </editor-fold>
}
2 Answers 2
You check that res must match a specific pattern, but name seems to be free game. What if a user calls this with values like:
../../../../etc/passwd../../../../../../home/jack/.ssh/id_rsa- ...
Avoid concatenating path segments with / separator. Let the two-argument constructor of File do that for you. If you have three path segments then use an intermediary file insurance.
You used try-with-resources nicely in processRequest but not in other places. It would be good to use that technique everywhere when working with closeable resources.
It's something of a nitpick, but you have
public void write(File file, BufferedOutputStream bufferedOutput) throws IOException { byte buffer[] = new byte[1024 * 4]; BufferedInputStream bufferedInput = null; try { FileInputStream inputStream = new FileInputStream(file); bufferedInput = new BufferedInputStream(inputStream); int lengthRead = 0; int offset = 0; while (true) { lengthRead = bufferedInput.read(buffer, 0, buffer.length); if (lengthRead == -1) { break; } bufferedOutput.write(buffer, 0, lengthRead); offset += lengthRead; } } finally { if (bufferedInput != null) { bufferedInput.close(); } } }
which you call
ServletOutputStream out = response.getOutputStream(); BufferedOutputStream bufferedOutputStream = new BufferedOutputStream(out); write(file, bufferedOutputStream); bufferedOutputStream.flush();
I personally find it confusing for that method to be called write when most of its logic is about reading from file. Perhaps change its name to echo and call it like
echo(file, response.getOutputStream());
Then the method itself would change to
public void echo(File in, ServletOutputStream out) throws IOException {
BufferedInputStream bufferedInput = null;
BufferedOutputStream bufferedOutput = null;
try {
byte buffer[] = new byte[1024 * 4];
bufferedOutputStream = new BufferedOutputStream(out);
FileInputStream inputStream = new FileInputStream(in);
bufferedInput = new BufferedInputStream(inputStream);
int lengthRead = bufferedInput.read(buffer, 0, buffer.length);
while (lengthRead != -1) {
bufferedOutput.write(buffer, 0, lengthRead);
lengthRead = bufferedInput.read(buffer, 0, buffer.length);
}
} finally {
if (bufferedInput != null) {
bufferedInput.close();
}
if (bufferedOutput != null) {
// we don't want to close the ServletOutputStream
// so we just flush here
bufferedOutput.flush();
}
}
}
It makes more sense to me to have a method that handles both input and output streams called echo. Also note that I moved more of the output related logic into the method. This puts the input and output operations on a more level footing.
Added a comment explaining why we call flush rather than close. Hopefully I followed the logic there correctly. If not, then I think that highlights why that decision should be commented.
I removed offset, as it was generated but never used.
I changed the while (true) to remove the break statement. It's a matter of personal preference, but I find this version easier to read. The other version is generally fine.
Another variant would be
int lengthRead = 0;
while ((lengthRead = bufferedInput.read(buffer, 0, buffer.length)) != -1) {
bufferedOutput.write(buffer, 0, lengthRead);
}
Which has the advantage that it avoids duplicate code. But I find that harder to follow, as it's rather busy.
-
\$\begingroup\$ Thanks for good refactoring proposal, added it to the corrected version! \$\endgroup\$Anton P. Kolosov– Anton P. Kolosov2015年06月19日 10:29:32 +00:00Commented Jun 19, 2015 at 10:29