Davide Trentin
e608977359
Static analysis findings for nc_program_manager: 8 security criticals (FTP plaintext, LSV2 cleartext, path traversal, license bypass), 4 functional bugs, 3 NRE risks, 3 resource leaks. See BUG_ANALYSIS.md for prioritized fix list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
120 lines
4.6 KiB
Markdown
120 lines
4.6 KiB
Markdown
# Analisi Bug & Problemi - nc_program_manager
|
|
|
|
Data: 2026-05-25
|
|
Scope: progetto `nc_program_manager` (esclusi test, obj/, Properties/)
|
|
|
|
## 🔴 Critici (security)
|
|
|
|
### FTP plaintext - credenziali esposte (no TLS)
|
|
- `Cnc/Fagor/FagorFtpClient.cs:60` - `ftp://` URI con NetworkCredential, nessun supporto SSL/TLS
|
|
- `Cnc/Mitsubishi/MitsubishiFtpClient.cs:60` - stesso pattern
|
|
- `Cnc/Siemens/SiemensFtpClient.cs:60` - stesso pattern
|
|
|
|
**Fix**: usare `FtpWebRequest.EnableSsl = true` o migrare a `ftps://` / SFTP. Se rete trusted, documentare requirement esplicito.
|
|
|
|
### LSV2 credenziali cleartext su TCP raw
|
|
- `Cnc/Heidenhain/Lsv2Client.cs:105` - username+password concatenati con null terminator, inviati non cifrati su socket TCP.
|
|
|
|
**Fix**: documentare come limitazione protocollo LSV2 oppure incapsulare in TLS tunnel.
|
|
|
|
### Path traversal - no validation prima FTP delete
|
|
- `Cnc/Fagor/FagorMachine.cs:121` - `DeleteProgramAsync` passa path utente direttamente a `_ftp.DeleteFile()`
|
|
- `Cnc/Mitsubishi/MitsubishiMachine.cs:117` - stesso pattern
|
|
- `Cnc/Siemens/SiemensMachine.cs:117` - stesso pattern
|
|
|
|
**Fix**: validare path (no `..`, no path absoluti fuori dir consentita) prima di chiamata FTP.
|
|
|
|
### License bypass in DEBUG build
|
|
- `Program.cs:47` - check licenza saltato in DEBUG conditional compile.
|
|
|
|
**Fix**: rimuovere `#if DEBUG` skip, o se necessario per dev limitare a `[Conditional]` log non funzionale.
|
|
|
|
### License read exception swallowed
|
|
- `Licensing/LicenseFile.cs:20` - errori lettura file licenza silenziosamente catturati, bypassano errori.
|
|
|
|
**Fix**: log + rethrow o ritorno result con error info.
|
|
|
|
---
|
|
|
|
## 🟠 Bug funzionali
|
|
|
|
### Program.Replace() return ignorato
|
|
- `FanucProgram.cs:176` - `Program.Replace(...)` chiamato senza assegnazione. String immutable, nessun effetto.
|
|
|
|
**Fix**: `Program = Program.Replace(...)`.
|
|
|
|
### Off-by-one in IsoProgramFormatter (commenti ISO)
|
|
- `Cnc/Fanuc/IsoProgramFormatter.cs:19` - `if (closePar > 0)` deve essere `>= 0` per gestire commento a inizio riga
|
|
- `Cnc/Fanuc/IsoProgramFormatter.cs:41` - `if (openPar > 0 && closePar > openPar)` deve essere `openPar >= 0`
|
|
|
|
**Fix**: cambiare `> 0` in `>= 0` quando si controlla risultato `IndexOf`.
|
|
|
|
### FanucDialectDetector - comparison string inconsistente
|
|
- `Cnc/Fanuc/FanucDialectDetector.cs:71` - `if (trimmed == "0" || t == " 0")` usa `t` non trimmato a destra mentre `trimmed` a sinistra. Edge case mancato.
|
|
|
|
**Fix**: uniformare uso di `trimmed` o documentare intent.
|
|
|
|
---
|
|
|
|
## 🟡 NullReferenceException
|
|
|
|
### ArgParser - Substring su null
|
|
- `ArgParser.cs:23` - `arguments.Find(...)` ritorna `null` se argomento `-azione=` mancante, poi `.Substring(8)` solleva NRE.
|
|
|
|
**Fix**: null check prima di Substring, o usare `?.Substring(8)` con default.
|
|
|
|
### FanucProgram - field Program nullable usato senza check
|
|
- `FanucProgram.cs:116` - `Standardize()` chiama `Program.IndexOf` / `Program.Replace` senza guardia null
|
|
- `FanucProgram.cs:188` - `ToString()` concatena `Program`, `ToolOffsetData`, `WorkZeroOffsetData` potenzialmente null
|
|
|
|
**Fix**: null check all'ingresso metodi o inizializzazione a `string.Empty` nel costruttore.
|
|
|
|
---
|
|
|
|
## 🟡 Resource leak / robustness
|
|
|
|
### FanucMachine - buffer access fragile
|
|
- `Cnc/Fanuc/FanucMachine.cs:407` - `buffer[len-1]` su `num` ritornato da FOCAS user-controlled. Se `len == UploadBufferSize + 1` raggiunge boundary.
|
|
|
|
**Fix**: clamp `len` su `Math.Min(len, buffer.Length)`.
|
|
|
|
### Lsv2Client - TcpClient/Stream non disposti se Login fallisce
|
|
- `Cnc/Heidenhain/Lsv2Client.cs:43` - `_stream` assegnato, poi `Login()` può throware; manca try/finally per cleanup.
|
|
|
|
**Fix**: wrap connect+login in try/catch con dispose esplicito su exception.
|
|
|
|
### Lsv2Client - ReadFrame ritorna dati parziali su EOF
|
|
- `Cnc/Heidenhain/Lsv2Client.cs:163` - loop esce su `n == 0` (EOF/disconnect) ma processa buffer incompleto, ritornando frame parziale.
|
|
|
|
**Fix**: throw o ritorno error se `n == 0` prima di lunghezza attesa.
|
|
|
|
---
|
|
|
|
## 🟢 Nit
|
|
|
|
### DateTime.Now per timestamp
|
|
- `Cnc/Models/CncError.cs:17` - `DateTime.Now` soggetto a drift/DST. Usare `DateTime.UtcNow` o `DateTimeOffset.UtcNow`.
|
|
|
|
---
|
|
|
|
## Riepilogo
|
|
|
|
| Severità | Count |
|
|
|----------|-------|
|
|
| Critici security | 8 |
|
|
| Bug funzionali | 4 |
|
|
| NRE | 3 |
|
|
| Resource/robustness | 3 |
|
|
| Nit | 1 |
|
|
| **Totale** | **19** |
|
|
|
|
## Priorità fix consigliata
|
|
|
|
1. TLS su FTP + LSV2 (o documenta requirement rete trusted)
|
|
2. Path validation prima delete FTP
|
|
3. Rimuovi DEBUG bypass licenza
|
|
4. Null checks su `FanucProgram.Program` e `ArgParser`
|
|
5. Fix off-by-one `IsoProgramFormatter`
|
|
6. Fix `FanucProgram.Replace()` assignment
|
|
7. Dispose pattern `Lsv2Client`
|
|
8. Logging exception `LicenseFile`
|