4
\$\begingroup\$

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>

}
\$\endgroup\$
0

2 Answers 2

10
\$\begingroup\$

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.

\$\endgroup\$
0
5
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for good refactoring proposal, added it to the corrected version! \$\endgroup\$ Commented Jun 19, 2015 at 10:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.