[Security issue] Unrestricted Upload

[gaellafond]

Anyone can upload any file to the server using a POST request to any GeoNetwork URL, without been logged. This issue is present in all version of GeoNetwork.

I discovered the issue after finding a IRC Botnet on our production server.

[josegar74]

Thanks for reporting this. It seems restricted to the upload directory and related to requests related not mapped Spring MVC (handled by jeeves).

I'm going to commit a fix for 3.0.x and 3.2.x, to cleanup the uploaded files at the end of the request.

Further improvements should be done to remove completely the jeeves requests in next versions and handle all with Spring MVC.

[gaellafond]

I made a quick patch for our instance of GeoNetwork. Rather than allowing unauthenticated user to upload files then deleting it, I simply prevent the upload in the first place. It's not perfect, the exception doesn't bubble up as expected. I end-up with a NPE in the browser when I try to upload a file while been unauthenticated, but at least it's safe. I will attempt to improve it.

Project: core

Class: jeeves.server.sources.ServiceRequestFactory

Method: getMultipartParams

Before

    //---------------------------------------------------------------------------

    private static Element getMultipartParams(AbstractMultipartHttpServletRequest req, Path uploadDir, int maxUploadSize) throws Exception {
        Element params = new Element("params");

        long maxSizeInBytes = maxUploadSize * 1024L * 1024L;
        convertParamToElem(req, params);
        final Map<String, MultipartFile> fileMap = req.getFileMap();
        for (Map.Entry<String, MultipartFile> fileEntry : fileMap.entrySet()) {
            final MultipartFile multipartFile = fileEntry.getValue();
            final long size = multipartFile.getSize();
            if (size > maxSizeInBytes) {
                throw new FileUploadTooBigEx();
            }
            String file = fileEntry.getValue().getOriginalFilename();
            final String type = multipartFile.getContentType();
            if (Log.isDebugEnabled(Log.REQUEST)) {
                Log.debug(Log.REQUEST, "Uploading file " + file + " type: " + type + " size: " + size);
            }

            //--- remove path information from file (some browsers put it, like IE)
            file = simplifyName(file);
            if (Log.isDebugEnabled(Log.REQUEST)) {
                Log.debug(Log.REQUEST, "File is called " + file + " after simplification");
            }
            multipartFile.transferTo(uploadDir.resolve(file).toFile());

After:

    // GAEL FIX
    private static User getAuthenticatedUser() {
        SecurityContext securityContext = SecurityContextHolder.getContext();
        if (securityContext != null) {
            Authentication authentication = securityContext.getAuthentication();
            if (authentication != null) {
                Object rawUser = authentication.getPrincipal();
                if (rawUser != null && (rawUser instanceof User)) {
                    return (User) rawUser;
                }
            }
        }
        return null;
    }

    //---------------------------------------------------------------------------

    private static Element getMultipartParams(AbstractMultipartHttpServletRequest req, Path uploadDir, int maxUploadSize) throws Exception {
        Element params = new Element("params");

        // GAEL FIX
        User loggedUser = getAuthenticatedUser();

        long maxSizeInBytes = maxUploadSize * 1024L * 1024L;
        convertParamToElem(req, params);
        final Map<String, MultipartFile> fileMap = req.getFileMap();
        for (Map.Entry<String, MultipartFile> fileEntry : fileMap.entrySet()) {
            final MultipartFile multipartFile = fileEntry.getValue();
            final long size = multipartFile.getSize();
            if (size > maxSizeInBytes) {
                throw new FileUploadTooBigEx();
            }
            String file = fileEntry.getValue().getOriginalFilename();
            final String type = multipartFile.getContentType();
            if (Log.isDebugEnabled(Log.REQUEST)) {
                Log.debug(Log.REQUEST, "Uploading file " + file + " type: " + type + " size: " + size);
            }

            //--- remove path information from file (some browsers put it, like IE)
            file = simplifyName(file);
            if (Log.isDebugEnabled(Log.REQUEST)) {
                Log.debug(Log.REQUEST, "File is called " + file + " after simplification");
            }
            // GAEL FIX
            if (loggedUser == null) {
                throw new IllegalAccessException("Attempt to upload a file without been logged.");
            }
            multipartFile.transferTo(uploadDir.resolve(file).toFile());

[fxprunayre]

Rather than allowing unauthenticated user to upload files then deleting it

BTW, Jose's fix allows to have anonymous user doing upload which might be required in some cases (eg. feedback with attachments - even if not available in default UI). Cleaning multipart request files is also the strategy adopted in Spring.

[gaellafond]

We don't want any more IRC Botnet, pornography, etc on our servers. If there is a feature that can be used to upload files anonymously, we don't want that feature.