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

Extra end of line in (M)LIST potentially breaks clients #64

Closed
buyology opened this issue Feb 12, 2018 · 2 comments
Closed

Extra end of line in (M)LIST potentially breaks clients #64

buyology opened this issue Feb 12, 2018 · 2 comments
Labels

Comments

@buyology
Copy link

buyology commented Feb 12, 2018

Using goftp to do a LIST on the sample ftpserver (both with the versions on tip), I discovered that these extra CRLFs result in parsing errors (also on L152 for MLIST):

https://github.com/fclairamb/ftpserver/blob/e240d7906ab840a805aabde615025b56ffbb4cf7/server/handle_dirs.go#L144

Unsure what the standard says — if this is something that the client should accept or if this should be fixed here?

Repro

conf := goftp.Config{User: "test", Password: "test"}

cli, err := goftp.DialConfig(conf, "localhost:2121")
if err != nil {
    t.Fatal(err)
}

_, err = cli.ReadDir("/")
if err != nil {
    t.Fatal(err)
}

yields

failed parsing MLST entry: 
@asv
Copy link
Collaborator

asv commented Feb 13, 2018

I can'n find a description of the LIST output format.... But proftpd does't add CRLF to the end of LIST-response.

[Need more research]

@buyology
Copy link
Author

buyology commented Feb 13, 2018

I'm looking at RFC3659 and the examples of MLST/MLSD responses look like this:

7.7.2. MLST of a directory

C> PWD
S> 257 "/" is current directory.
C> MLst tmp
S> 250- Listing tmp
S>  Type=dir;Modify=19981107085215;Perm=el; /tmp
S> 250 End

7.7.3. MLSD of a directory

C> MLSD tmp
S> 150 BINARY connection open for MLSD tmp
D> Type=cdir;Modify=19981107085215;Perm=el; tmp
D> Type=cdir;Modify=19981107085215;Perm=el; /tmp
D> Type=pdir;Modify=19990112030508;Perm=el; ..
D> Type=file;Size=25730;Modify=19940728095854;Perm=; capmux.tar.z
D> Type=file;Size=1830;Modify=19940916055648;Perm=r; hatch.c
D> Type=file;Size=25624;Modify=19951003165342;Perm=r; MacIP-02.txt
D> Type=file;Size=2154;Modify=19950501105033;Perm=r; uar.netbsd.patch
D> Type=file;Size=54757;Modify=19951105101754;Perm=r; iptnnladev.1.0.sit.hqx
D> Type=file;Size=226546;Modify=19970515023901;Perm=r; melbcs.tif
D> Type=file;Size=12927;Modify=19961025135602;Perm=r; tardis.1.6.sit.hqx
D> Type=file;Size=17867;Modify=19961025135602;Perm=r; timelord.1.4.sit.hqx
D> Type=file;Size=224907;Modify=19980615100045;Perm=r; uar.1.2.3.sit.hqx
D> Type=file;Size=1024990;Modify=19980130010322;Perm=r; cap60.pl198.tar.gz
S> 226 MLSD completed

and the spec says:

      mlst-response    = control-response / error-response
      mlsd-response    = ( initial-response final-response ) /
                         error-response

      control-response = "250-" [ response-message ] CRLF
                         1*( SP entry CRLF )
                         "250" [ SP response-message ] CRLF

      initial-response = "150" [ SP response-message ] CRLF
      final-response   = "226" SP response-message CRLF

      response-message = *TCHAR

      data-response    = *( entry CRLF )

      entry            = [ facts ] SP pathname
      facts            = 1*( fact ";" )
      fact             = factname "=" value
      factname         = "Size" / "Modify" / "Create" /
                         "Type" / "Unique" / "Perm" /
                         "Lang" / "Media-Type" / "CharSet" /
                         os-depend-fact / local-fact
      os-depend-fact   = <IANA assigned OS name> "." token

and that seems to also indicate that there should be no extra CRLF.

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

No branches or pull requests

3 participants