Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get rid of proc_open #379

Closed
damnms opened this issue Aug 13, 2020 · 9 comments
Closed

get rid of proc_open #379

damnms opened this issue Aug 13, 2020 · 9 comments

Comments

@damnms
Copy link

damnms commented Aug 13, 2020

i would like to secure my php.ini, but i can not restrict it to deny proc_open because the current code base requires the execution of a filesystem application (doveadm).
that also blocks the clean* containerization of postfixadmin.

*) one application in one container, currently only possible with 2 (postfixadmin and dovecot)

@DavidGoodwin
Copy link
Member

Do you have any suggestions as to what we could replace proc_open with ?

Is a call to 'exec' better?

# Use proc_open call to avoid safe_mode problems and to prevent showing plain password in process table

@damnms
Copy link
Author

damnms commented Aug 18, 2020

no, i would replace the calls to filesystem binaries at all. it makes no difference if exec/proc_open/system etc. are called, they are (more or less) the same.

if that would be java, i would help you, but i am really no php programmer.

sha and all the other hashing algorithms are supported by php, so i would use those.
that class is already very very big, i guess it would make sense to extract all that hashing stuff to another class.
is there a reason why you rely on the doveadm binary?

why not create a new class like hmm...

this is some pseudocode how i would create that

public class DovecotEncryptionMapper {

  private final String dovecotUsedString;

  public DovecotEncryptionMapper(String dovecotUsedString) {
    this.dovecotUsedString = dovecotUsedString; //e.g. dovecot:digest-md5
  }
  
  private String getPHPizedAlgorithm(String dovecotUsedString) {

    switch(dovecotUsedString) {
    case: "dovecot:digest-md5": throw new RuntimeException("not supported");
    case: "dovecot:ripemd160": return "ripemd160"; break;
    ... and so on
    default: throw new RuntimeException("algorithm " + dovecotUsedString " + currently not supported, please report this");
    }
  }

  //that is not possible in java but in php
  public String getEncrypted(String theStringToHash) {
    return call_user_func("hash",getPHPizedAlgorithm(),theStringToHash);
  }
}

@damnms
Copy link
Author

damnms commented Aug 18, 2020

so the only thing you have to do in the future is to add 1 line if dovecot adds new support for another hashing algorithm, and its easy testable and you can use it everywhere where dovecot and hashing is used

@damnms
Copy link
Author

damnms commented Aug 18, 2020

at the moment i have some time... its a long time ago since i did php, but i will set up everything tomorrow.
then i try to fix that how i would do it and create a PR. but it might take some time because... php was long ago for me and i need to google a lot for that ;)
except you say now "hey thats a good approach, i can do that", then just tell me and i dont try to fix that :)

@damnms
Copy link
Author

damnms commented Aug 18, 2020

not sure if my approach is good for your project because i would try to do that in an object oriented way, and the functions.php looks more like a procedural coding style. is there any plan to move more to a object oriented style or do you want to stay procedural?

@DavidGoodwin
Copy link
Member

Right - i think it's been a case of :

  • "Why write all this code to do something that dovecot does already" and
  • "Using dovecot we're guaranteed to get the right hashes / don't have to worry about messing up the encryption / don't have to support it ... ".

Over time different people have added additional hash support.

I agree the functions.inc.php file is too big, but there are loads of things that need fixing with the project (for me: I find the Handler classes difficult to use/follow and overly complex, there aren't enough tests, the code isn't extensible enough to support things like TOTP etc)......

@damnms
Copy link
Author

damnms commented Aug 19, 2020

relying on binaries is always bad.
for exactly that reason in http context, there is libcurl.
it has reasons why dev's should create libraries if they provide something "new" or so complex, so others can use their libraries and create binaries and not depend on binaries.
not depending on binaries gives much more flexibility, testability, etc.

"Why write all this code to do something that dovecot does already" and

yes if you want to parse the output. lets assume dovecot changes their output to something different, then its incompatible.
assume an evil hacker has somehow access to doveadm, manipulates it and makes something like "rm -rf /" in it.
assume you want to run postfixadmin in a container, thats impossible without installing dovecot which breaks the "1-container-1-app"-rule, assume you want to be more OS independent, ... just to mention a few aspects.

"Using dovecot we're guaranteed to get the right hashes / don't have to worry about messing up the encryption / don't have to support it ... ".

thats absolutely wrong. imagine you have arch linux with dovecot 2.x and fedora with dovecot 3.x - thats a breaking change. YOU need to test if it works or not when you rely on a binary. then its required to have some kind of compatibility matrix.
you dont have to support or worry about hashing, you just have to pass it not to doveadm but to the php function "sha", thats it.

making sure "hello world" gives the correct hash should be more or less a oneliner in phpunit for each hashing algorithm.

@DavidGoodwin
Copy link
Member

#491

@damnms
Copy link
Author

damnms commented May 9, 2021

thanks for your effort! :)
what a pity that it wont make it into debian 11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants