Skip to content

Commit

Permalink
hide thumbnails behind random path (fixes #18)
Browse files Browse the repository at this point in the history 
A security review of the thumbnail component revealed complete user
data exposure of a remotes content when browsing that remote while
having thumbnails enabled. For details see security notice.

Signed-off-by: x0b <[email protected]>
  • Loading branch information
@x0b
x0b committed Oct 4, 2019
1 parent bc2b012 commit b5bb5a1
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 21 deletions.
5 changes: 5 additions & 0 deletions app/src/main/java/ca/pkay/rcloneexplorer/Dialogs/RemoteDestinationDialog.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,11 @@ public void onFileOptionsClicked(View view, FileItem fileItem) {
// don't do anything
}

@Override
public String[] getThumbnailServerParams() {
return new String[0];
}

private void onCreateNewDirectory() {
if (getFragmentManager() != null) {
new InputDialog()
Expand Down
17 changes: 17 additions & 0 deletions app/src/main/java/ca/pkay/rcloneexplorer/Fragments/FileExplorerFragment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import android.preference.PreferenceManager;
import android.text.Editable;
import android.text.TextWatcher;
import android.util.Base64;
import android.util.TypedValue;
import android.view.LayoutInflater;
import android.view.Menu;
Expand Down Expand Up @@ -77,6 +78,7 @@
import java.io.IOException;
import java.net.HttpURLConnection;
import java.net.URL;
import java.security.SecureRandom;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -151,6 +153,7 @@ public class FileExplorerFragment extends Fragment implements FileExplorerRecy
private boolean startAtRoot;
private boolean goToDefaultSet;
private Context context;
private String thumbnailServerAuth;

/**
* Mandatory empty constructor for the fragment manager to instantiate the
Expand Down Expand Up @@ -237,6 +240,7 @@ public View onCreateView(@NonNull LayoutInflater inflater, @Nullable ViewGroup c
}

if (showThumbnails) {
initializeThumbnailParams();
startThumbnailService();
}

Expand Down Expand Up @@ -731,10 +735,18 @@ private void startThumbnailService() {
}
Intent serveIntent = new Intent(getContext(), ThumbnailsLoadingService.class);
serveIntent.putExtra(ThumbnailsLoadingService.REMOTE_ARG, remote);
serveIntent.putExtra(ThumbnailsLoadingService.HIDDEN_PATH, thumbnailServerAuth);
context.startService(serveIntent);
isThumbnailsServiceRunning = true;
}

private void initializeThumbnailParams() {
SecureRandom random = new SecureRandom();
byte[] values = new byte[16];
random.nextBytes(values);
thumbnailServerAuth = Base64.encodeToString(values, Base64.NO_PADDING | Base64.NO_WRAP | Base64.URL_SAFE);
}

private void searchClicked() {
if (isSearchMode) {
if (!is720dp) {
Expand Down Expand Up @@ -1286,6 +1298,11 @@ public void onFileOptionsClicked(View view, FileItem fileItem) {
showFileMenu(view, fileItem);
}

@Override
public String[] getThumbnailServerParams() {
return new String[]{thumbnailServerAuth};
}

private void showFileMenu(View view, final FileItem fileItem) {
PopupMenu popupMenu = new PopupMenu(context, view);
popupMenu.getMenuInflater().inflate(R.menu.file_explorer_menu, popupMenu.getMenu());
Expand Down
5 changes: 5 additions & 0 deletions app/src/main/java/ca/pkay/rcloneexplorer/Fragments/ShareFragment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,11 @@ public void onFileOptionsClicked(View view, FileItem fileItem) {
// Don't do anything
}

@Override
public String[] getThumbnailServerParams() {
return new String[0];
}

@Override
public void onBreadCrumbClicked(String path) {
if (directoryObject.getCurrentPath().equals(path)) {
Expand Down
50 changes: 37 additions & 13 deletions app/src/main/java/ca/pkay/rcloneexplorer/Rclone.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import android.util.Log;
import android.webkit.MimeTypeMap;
import android.widget.Toast;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import ca.pkay.rcloneexplorer.Items.FileItem;
import ca.pkay.rcloneexplorer.Items.RemoteItem;
Expand All @@ -25,9 +26,11 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.InterruptedIOException;
import java.io.OutputStream;
import java.io.OutputStreamWriter;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
Expand Down Expand Up @@ -122,8 +125,14 @@ public void logErrorOutput(Process process) {
while ((line = reader.readLine()) != null) {
stringBuilder.append(line).append("\n");
}
} catch (InterruptedIOException iioe) {
Log.i(TAG, "logErrorOutput: process died while reading. Log may be incomplete.");
} catch (IOException e) {
Log.e(TAG, "logErrorOutput: ", e);
if("Stream closed".equals(e.getMessage())) {
Log.d(TAG, "logErrorOutput: could not read stderr, process stream is already closed");
} else {
Log.e(TAG, "logErrorOutput: ", e);
}
return;
}
log2File.log(stringBuilder.toString());
Expand Down Expand Up @@ -384,7 +393,9 @@ public String obscure(String pass) {
}
}

public Process serve(int protocol, int port, boolean allowRemoteAccess, String user, String password, RemoteItem remote, String servePath) {
public Process serve(int protocol, int port, boolean allowRemoteAccess, @Nullable String user,
@Nullable String password, @NonNull RemoteItem remote, @Nullable String servePath,
@Nullable String baseUrl) {
String remoteName = remote.getName();
String localRemotePath = (remote.isRemoteType(RemoteItem.LOCAL)) ? Environment.getExternalStorageDirectory().getAbsolutePath() + "/" : "";
String path = (servePath.compareTo("//" + remoteName) == 0) ? remoteName + ":" + localRemotePath : remoteName + ":" + localRemotePath + servePath;
Expand All @@ -410,18 +421,25 @@ public Process serve(int protocol, int port, boolean allowRemoteAccess, String u
String cachePath = context.getCacheDir().getAbsolutePath();
String[] environmentalVariables = {"TMPDIR=" + cachePath}; // this is a fix for #199

String[] command;
ArrayList<String> params = new ArrayList<>(Arrays.asList(
createCommandWithOptions("serve", commandProtocol, "--addr", address, path)));

if (user == null && password != null) {
command = createCommandWithOptions("serve", commandProtocol, "--addr", address, path, "--pass", password);
} else if (user != null && password == null) {
command = createCommandWithOptions("serve", commandProtocol, "--addr", address, path, "--user", user);
} else if (user != null) {
command = createCommandWithOptions("serve", commandProtocol, "--addr", address, path, "--user", user, "--pass", password);
} else {
command = createCommandWithOptions("serve", commandProtocol, "--addr", address, path);
if(null != user) {
params.add("--user");
params.add(user);
}

if(null != password) {
params.add("--password");
params.add(password);
}

if(null != baseUrl) {
params.add("--baseurl");
params.add(baseUrl);
}

String[] command = params.toArray(new String[0]);
try {
if (protocol == SERVE_PROTOCOL_WEBDAV) {
return Runtime.getRuntime().exec(command, environmentalVariables);
Expand All @@ -434,8 +452,14 @@ public Process serve(int protocol, int port, boolean allowRemoteAccess, String u
}
}

public Process serve(int protocol, int port, boolean localhostOnly, RemoteItem remote, String servePath) {
return serve(protocol, port, localhostOnly, null, null, remote, servePath);
// TODO: remove for 1.9.1
// Disabled, this is a security issue
//public Process serve(int protocol, int port, boolean localhostOnly, RemoteItem remote, String servePath) {
// return serve(protocol, port, localhostOnly, null, null, remote, servePath);
//}

public Process serve(int protocol, int port, boolean allowRemoteAccess, String user, String password, RemoteItem remote, String servePath) {
return serve(protocol, port, allowRemoteAccess, user, password, remote, servePath, null);
}

public Process sync(RemoteItem remoteItem, String remote, String localPath, int syncDirection) {
Expand Down
8 changes: 6 additions & 2 deletions ...ain/java/ca/pkay/rcloneexplorer/RecyclerViewAdapters/FileExplorerRecyclerViewAdapter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public interface OnClickListener {
void onFilesSelected();
void onFileDeselected();
void onFileOptionsClicked(View view, FileItem fileItem);
String[] getThumbnailServerParams();
}

public FileExplorerRecyclerViewAdapter(Context context, View emptyView, View noSearchResultsView, OnClickListener listener) {
Expand Down Expand Up @@ -98,7 +99,7 @@ public void onBindViewHolder(@NonNull final ViewHolder holder, int position) {
}

if (showThumbnails && !item.isDir()) {
String server = "http://127.0.0.1:29170/";
String server = "http://127.0.0.1:29179/";
boolean localLoad = item.getRemote().getType() == RemoteItem.SAFW;
String mimeType = item.getMimeType();
if ((mimeType.startsWith("image/") || mimeType.startsWith("video/")) && item.getSize() <= 20970000) {
Expand All @@ -114,9 +115,12 @@ public void onBindViewHolder(@NonNull final ViewHolder holder, int position) {
.thumbnail(0.1f)
.into(holder.fileIcon);
} else {
String[] serverParams = listener.getThumbnailServerParams();
String hiddenPath = serverParams[0];
String url = server + hiddenPath + '/' + item.getPath();
Glide
.with(context)
.load(server + item.getPath())
.load(url)
.apply(glideOption)
.thumbnail(0.1f)
.into(holder.fileIcon);
Expand Down
19 changes: 13 additions & 6 deletions app/src/main/java/ca/pkay/rcloneexplorer/Services/ThumbnailsLoadingService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@
import android.app.IntentService;
import android.content.Intent;
import androidx.annotation.Nullable;

import ca.pkay.rcloneexplorer.Items.RemoteItem;
import ca.pkay.rcloneexplorer.R;
import ca.pkay.rcloneexplorer.Rclone;

public class ThumbnailsLoadingService extends IntentService {

public static final String REMOTE_ARG = "ca.pkay.rcexplorer.ThumbnailsLoadingService.REMOTE_ARG";
public static final String HIDDEN_PATH = "ca.pkay.rcexplorer.ThumbnailsLoadingService.HIDDEN_PATH";
private Rclone rclone;
private Process process;

Expand All @@ -30,18 +31,24 @@ protected void onHandleIntent(@Nullable Intent intent) {
}

RemoteItem remote = intent.getParcelableExtra(REMOTE_ARG);
process = rclone.serve(Rclone.SERVE_PROTOCOL_HTTP, 29170, true, remote, "");
String hiddenPath = "/" + intent.getStringExtra(HIDDEN_PATH);
process = rclone.serve(Rclone.SERVE_PROTOCOL_HTTP, 29179, false, null, null, remote, "", hiddenPath);
if (process != null) {
try {
if(getSharedPreferences(getPackageName() + "_preferences", MODE_PRIVATE).
getBoolean(getString(R.string.pref_key_logs), false)) {
new Thread() {
@Override
public void run() {
rclone.logErrorOutput(process);
}
}.start();
}
process.waitFor();
} catch (InterruptedException e) {
e.printStackTrace();
}
}

if (process != null && process.exitValue() != 0) {
rclone.logErrorOutput(process);
}
}

@Override
Expand Down

0 comments on commit b5bb5a1

Please sign in to comment.